SSR #196
SSR #196
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
WalkthroughThis pull request introduces significant changes to improve code maintainability and readability. It includes the transition from relative to absolute import paths, refactoring of components, introduction of language settings, and replacement of Apollo Client with a custom GQty client for GraphQL queries. Changes
Uplevel your code reviews with CodeRabbit ProCodeRabbit ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (29)
- .gitignore (2)
- app/chapter/[chapterNumber]/(chapter)/chapter-page.tsx (3)
- app/chapter/[chapterNumber]/(chapter)/layout.tsx (1)
- app/chapter/[chapterNumber]/(chapter)/page.tsx (1)
- app/chapter/[chapterNumber]/verse/[verseNumber]/page.tsx (2)
- app/chapter/[chapterNumber]/verse/[verseNumber]/verse-page.tsx (2)
- app/chapter/[chapterNumber]/verse/layout.tsx (1)
- app/home-page.tsx (1)
- app/layout.tsx (2)
- app/providers.tsx (1)
- app/search/search-page.tsx (1)
- app/shared/constants.ts (1)
- app/shared/functions.ts (1)
- app/verse-of-the-day/verse-of-the-day.tsx (4)
- components/Headers/AudioPlayer.tsx (1)
- components/Headers/DarkModeToggle.tsx (2)
- components/Headers/PageHeader.tsx (13)
- components/Home/BackgroundImage.tsx (1)
- components/Home/Newsletter.tsx (3)
- components/Home/VerseOfDay.tsx (1)
- components/Shared/Author.tsx (6)
- components/Verse/Translation.tsx (1)
- components/Verse/Verse.tsx (2)
- hooks/useMyStyles.ts (1)
- lib/getVerseData.ts (2)
- middleware.ts (1)
- next.config.js (1)
- types.d.ts (2)
- utils/supabase.ts (1)
Files ignored due to filter (5)
- constant/commentary_authors.json
- constant/translation_authors.json
- package.json
- tsconfig.json
- yarn.lock
Files skipped from review due to trivial changes (19)
- .gitignore
- app/chapter/[chapterNumber]/(chapter)/chapter-page.tsx
- app/chapter/[chapterNumber]/(chapter)/layout.tsx
- app/chapter/[chapterNumber]/(chapter)/page.tsx
- app/chapter/[chapterNumber]/verse/[verseNumber]/verse-page.tsx
- app/chapter/[chapterNumber]/verse/layout.tsx
- app/home-page.tsx
- app/providers.tsx
- app/search/search-page.tsx
- components/Headers/AudioPlayer.tsx
- components/Headers/DarkModeToggle.tsx
- components/Headers/PageHeader.tsx
- components/Home/BackgroundImage.tsx
- components/Verse/Translation.tsx
- components/Verse/Verse.tsx
- hooks/useMyStyles.ts
- lib/getVerseData.ts
- next.config.js
- types.d.ts
Review comments generated (34)
- Review: 21
- LGTM: 13
Tips
Chat with 
CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
app/layout.tsx
Outdated
| @@ -6,6 +6,7 @@ import { Metadata } from "next"; | |||
| import { Inter } from "next/font/google"; | |||
| import Providers from "./providers"; | |||
| import { PreloadResources } from "./preload-resources"; | |||
| import classNames from "utils/classNames"; | |||
There was a problem hiding this comment.
The import of classNames is not used in the provided code. If it's not used elsewhere in the file, consider removing it to keep the code clean and maintainable.
- import classNames from "utils/classNames";
app/shared/constants.ts
Outdated
| import languages from "constant/languages.json"; | ||
| import commentaryAuthors from "constant/commentary_authors.json"; | ||
| import translationAuthors from "constant/translation_authors.json"; |
There was a problem hiding this comment.
The import paths for languages.json, commentary_authors.json, and translation_authors.json seem to be incorrect. The correct path should be relative and start with ./ or ../ depending on the file structure.
- import languages from "constant/languages.json";
- import commentaryAuthors from "constant/commentary_authors.json";
- import translationAuthors from "constant/translation_authors.json";
+ import languages from "./constant/languages.json";
+ import commentaryAuthors from "./constant/commentary_authors.json";
+ import translationAuthors from "./constant/translation_authors.json";
app/shared/constants.ts
Outdated
| export const defaultAdvancedSettings: AdvancedSettings = { | ||
| devnagari: true, | ||
| verseText: true, | ||
| synonyms: true, | ||
| translation: true, | ||
| purport: true, | ||
| }; |
There was a problem hiding this comment.
The AdvancedSettings type is not imported in this file, which could lead to a reference error. Please ensure that it's imported correctly.
+ import { AdvancedSettings } from 'path-to-advanced-settings';
export const defaultAdvancedSettings: AdvancedSettings = {
devnagari: true,
verseText: true,
synonyms: true,
translation: true,
purport: true,
};
components/Home/VerseOfDay.tsx
Outdated
| const VerseOfDay = async () => { | ||
| const dailyVerse = await getDailyVerse(); |
There was a problem hiding this comment.
The VerseOfDay component has been changed from a regular function to an async function. However, in React, components cannot be asynchronous. Async functions return a promise, and React components need to return a renderable output. This will cause an error when trying to render the VerseOfDay component.
- const VerseOfDay = async () => {
- const dailyVerse = await getDailyVerse();
+ const VerseOfDay = () => {
+ const [dailyVerse, setDailyVerse] = useState<GitaVerse | null>(null);
+
+ useEffect(() => {
+ const getVerseOfTheDay = async () => {
+ const verse = await getDailyVerse();
+ setDailyVerse(verse);
+ };
+
+ if (!dailyVerse) {
+ getVerseOfTheDay();
+ }
+ }, [dailyVerse]);
components/Shared/Author.tsx
Outdated
| import { useCookies } from "react-cookie"; | ||
|
|
||
| import { useState, Fragment, useEffect } from "react"; | ||
| import { Dialog, Transition, Switch, Listbox } from "@headlessui/react"; | ||
| import { SelectorIcon } from "@heroicons/react/solid"; | ||
| import languages from "../../constant/languages.json"; //todo: use graphql api to fetch | ||
| import commentary_authors from "../../constant/commentary_authors.json"; | ||
| import translation_authors from "../../constant/translation_authors.json"; | ||
| import { getLanguageSettings } from "app/shared/functions"; | ||
| import classNames from "utils/classNames"; | ||
|
|
||
| const cookieOptions = { path: "/", maxAge: 365 * 24 * 60 * 60 }; | ||
|
|
||
| interface Props { | ||
| authorSettingsIsOpen: boolean; | ||
| closeAuthorSettingsModal: () => void; | ||
| languageSettings: LanguageSettings; | ||
| setLanguageSettings: Dispatch<SetStateAction<LanguageSettings>>; | ||
| } | ||
|
|
||
| const Author = ({ | ||
| authorSettingsIsOpen, | ||
| closeAuthorSettingsModal, | ||
| languageSettings, | ||
| setLanguageSettings, | ||
| }: Props) => { | ||
| const Author = ({ authorSettingsIsOpen, closeAuthorSettingsModal }: Props) => { | ||
| const languageSettings = getLanguageSettings(); | ||
|
|
||
| const [isVerseCommentarySourceEnabled, setIsVerseCommentarySourceEnabled] = | ||
| useState(true); | ||
| const [ | ||
| isVerseTransliterationLanguageEnabled, | ||
| setIsVerseTransliterationLanguageEnabled, | ||
| ] = useState(true); | ||
| const [isVerseTranslationSourceEnabled, setIsVerseTranslationSourceEnabled] = | ||
| useState(true); | ||
| const [isSubmitting, setIsSubmitting] = useState(false); | ||
|
|
||
| const [cookies, setCookie] = useCookies([ | ||
| "languageId", | ||
| "translationAuthorId", | ||
| "commentaryAuthorId", | ||
| ]); | ||
|
|
||
| const [language, setLanguage] = useState(languageSettings.language); | ||
| const [translationAuthor, setTranslationAuthor] = useState({ | ||
| id: 0, | ||
| name: "", | ||
| language: "", | ||
| }); | ||
| const [commentaryAuthor, setCommentaryAuthor] = useState({ | ||
| id: 0, | ||
| name: "", | ||
| language: "", | ||
| }); | ||
| const [commentaryAuthor, setCommentaryAuthor] = useState({ id: 0, name: "" }); | ||
|
|
||
| useEffect(() => { | ||
| setLanguage(languageSettings.language); | ||
| setTranslationAuthor(languageSettings.translationAuthor); | ||
| setCommentaryAuthor(languageSettings.commentaryAuthor); | ||
| const myLanguageSettings = getLanguageSettings({ | ||
| languageId: parseInt(String(cookies.languageId)), | ||
| translationAuthorId: parseInt(String(cookies.translationAuthorId)), | ||
| commentaryAuthorId: parseInt(String(cookies.commentaryAuthorId)), | ||
| }); | ||
|
|
||
| setLanguage(myLanguageSettings.language); | ||
| setTranslationAuthor(myLanguageSettings.translationAuthor); | ||
| setCommentaryAuthor(myLanguageSettings.commentaryAuthor); | ||
| }, [ | ||
| languageSettings.commentaryAuthor, | ||
| languageSettings.language, | ||
| languageSettings.translationAuthor, | ||
| cookies.commentaryAuthorId, | ||
| cookies.languageId, | ||
| cookies.translationAuthorId, | ||
| ]); | ||
|
|
||
| function handleSubmit() { | ||
| setLanguageSettings({ | ||
| language: language, | ||
| translationAuthor: translationAuthor, | ||
| commentaryAuthor: commentaryAuthor, | ||
| }); | ||
| closeAuthorSettingsModal(); | ||
| setIsSubmitting(true); | ||
| setCookie("languageId", language.id, cookieOptions); | ||
| setCookie("translationAuthorId", translationAuthor.id, cookieOptions); | ||
| setCookie("commentaryAuthorId", commentaryAuthor.id, cookieOptions); | ||
| window.location.reload(); |
There was a problem hiding this comment.
The new implementation of the Author component seems to be handling cookies for storing user preferences. However, it's important to note that this approach might not work if the user has disabled cookies in their browser settings. It would be better to handle such cases gracefully and provide a fallback mechanism or at least inform the user about the necessity of enabling cookies for a better user experience.
Also, the window.location.reload() on line 67 is a hard refresh which could lead to a poor user experience as it will reload the entire page and all resources. If you want to re-render components based on the new state, consider using React's state and props to trigger a re-render of components.
middleware.ts
Outdated
| export function middleware(req: NextRequest) { | ||
| const { origin, pathname, searchParams } = new URL(req.url); | ||
|
|
||
| const cookieL = req.cookies.get("languageId")?.value; | ||
| const cookieT = req.cookies.get("translationAuthorId")?.value; | ||
| const cookieC = req.cookies.get("commentaryAuthorId")?.value; | ||
|
|
||
| const searchL = searchParams.get("l"); | ||
| const searchT = searchParams.get("t"); | ||
| const searchC = searchParams.get("c"); | ||
|
|
||
| const anyL = searchL || (typeof cookieL === "string" && cookieL) || ""; | ||
| const anyT = searchT || (typeof cookieT === "string" && cookieT) || ""; | ||
| const anyC = searchC || (typeof cookieC === "string" && cookieC) || ""; |
There was a problem hiding this comment.
The code is checking if the cookies are of type string and then assigning them to anyL, anyT, and anyC. However, it's not necessary to check if the cookie value is a string because the get method of cookies always returns a string or undefined. This can be simplified.
- const anyL = searchL || (typeof cookieL === "string" && cookieL) || "";
- const anyT = searchT || (typeof cookieT === "string" && cookieT) || "";
- const anyC = searchC || (typeof cookieC === "string" && cookieC) || "";
+ const anyL = searchL || cookieL || "";
+ const anyT = searchT || cookieT || "";
+ const anyC = searchC || cookieC || "";
middleware.ts
Outdated
|
|
||
| const someSettingIsInconsistent = | ||
| languageIsInconsistent || | ||
| translationAuthorIsInconsistent || | ||
| commentaryAuthorIsInconsistent; | ||
|
|
||
| if (someSettingIsInconsistent) { | ||
| const urlSearchParams = new URLSearchParams({ | ||
| l: String(language.id), | ||
| t: String(translationAuthor.id), | ||
| c: String(commentaryAuthor.id), | ||
| }); | ||
| const updatedUrl = `${origin}${pathname}?${urlSearchParams.toString()}`; | ||
| return NextResponse.redirect(updatedUrl); | ||
| } |
There was a problem hiding this comment.
The logic here checks if there is an inconsistency between the search parameters and the language settings. If there is, it redirects to an updated URL with the correct settings. However, this could potentially lead to an infinite loop if getLanguageSettings keeps returning inconsistent values. It would be better to add some error handling or a maximum redirect limit to prevent this.
middleware.ts
Outdated
|
|
||
| const response = NextResponse.next({ | ||
| headers: { | ||
| "x-settings-l": String(language.id), | ||
| "x-settings-t": String(translationAuthor.id), | ||
| "x-settings-c": String(commentaryAuthor.id), | ||
| }, | ||
| }); | ||
| return response; |
There was a problem hiding this comment.
The response headers are being set with the language settings. This seems fine, but it's important to ensure that these headers are used appropriately in the rest of the application and don't introduce any security vulnerabilities.
app/shared/functions.ts
Outdated
| subHeading1: "text-3xl", | ||
| subHeading2: "text-2xl", | ||
| para: "text-xl", | ||
| }, | ||
| lineHeight: "leading-none", | ||
| backgroundColor: "light-bg", | ||
| }; | ||
|
|
||
| export const getMyStyles = () => { | ||
| const myStyles = JSON.parse(JSON.stringify(defaultStyles)); | ||
|
|
||
| const fontSize = getCookie("fontSize"); | ||
| const spacing = getCookie("spacing"); | ||
| const bg = getCookie("bg"); | ||
|
|
||
| if (fontSize === "large") { | ||
| myStyles.fontSize = { | ||
| heading: "text-4xl", | ||
| subHeading1: "text-intro", | ||
| subHeading2: "text-paragraph", | ||
| para: "text-paragraph", | ||
| }; | ||
| } else if (fontSize == "small") { | ||
| myStyles.fontSize = { | ||
| heading: "text-3xl", | ||
| subHeading1: "text-2xl", | ||
| subHeading2: "text-xl", | ||
| para: "text-md", | ||
| }; | ||
| } | ||
|
|
||
| if (spacing === "large") { | ||
| myStyles.lineHeight = "leading-loose"; | ||
| } else if (spacing === "medium") { | ||
| myStyles.lineHeight = "leading-normal"; | ||
| } else if (spacing === "small") { | ||
| myStyles.lineHeight = "leading-none"; | ||
| } | ||
|
|
||
| if (bg === "bg-dark-bg") { | ||
| myStyles.backgroundColor = "dark-bg"; | ||
| } else if (bg === "bg-light-bg") { | ||
| myStyles.backgroundColor = "white"; | ||
| } else if (bg === "bg-yellow-bg") { | ||
| myStyles.backgroundColor = "yellow-bg"; | ||
| } | ||
|
|
||
| return myStyles; | ||
| }; | ||
|
|
||
| export const getMyLanguage = () => { | ||
| const languageId = getCookie("languageId") || 1; | ||
| return ( | ||
| languages.find(({ id }) => String(id) === String(languageId)) || | ||
| languages[0] | ||
| ); | ||
| }; | ||
|
|
||
| export const getMyCommentaryAuthor = () => { | ||
| const commentaryAuthorId = getCookie("commentaryAuthorId") || 16; | ||
| return ( | ||
| commentaryAuthors.find( | ||
| ({ id }) => String(id) === String(commentaryAuthorId) | ||
| ) || commentaryAuthors[0] | ||
| ); | ||
| }; | ||
|
|
||
| export const getMyTranslationAuthor = () => { | ||
| const translationAuthorId = getCookie("translationAuthorId") || 16; | ||
| return ( | ||
| translationAuthors.find( | ||
| ({ id }) => String(id) === String(translationAuthorId) | ||
| ) || translationAuthors[0] | ||
| ); | ||
| }; | ||
|
|
||
| export const getLanguageById = (languageId: number) => | ||
| languages.find(({ id }) => id === languageId); | ||
|
|
||
| export const getTranslationAuthorById = (translationAuthorId: number) => | ||
| translationAuthors.find(({ id }) => id === translationAuthorId); | ||
|
|
||
| export const getCommentaryAuthorById = (commentaryAuthorId: number) => | ||
| commentaryAuthors.find(({ id }) => id === commentaryAuthorId); | ||
|
|
||
| export const getLanguageSettings = ( | ||
| { languageId, translationAuthorId, commentaryAuthorId } = { | ||
| languageId: defaultLanguageId, | ||
| translationAuthorId: defaultTranslationAuthorId, | ||
| commentaryAuthorId: defaultCommentaryAuthorId, | ||
| } | ||
| ): LanguageSettings => ({ | ||
| language: getLanguageById(languageId) || getLanguageById(defaultLanguageId)!, | ||
| translationAuthor: | ||
| getTranslationAuthorById(translationAuthorId) || | ||
| getTranslationAuthorById(defaultTranslationAuthorId)!, | ||
| commentaryAuthor: | ||
| getCommentaryAuthorById(commentaryAuthorId) || | ||
| getCommentaryAuthorById(defaultCommentaryAuthorId)!, | ||
| }); |
There was a problem hiding this comment.
The getMyStyles, getMyLanguage, getMyCommentaryAuthor, and getMyTranslationAuthor functions are using the getCookie function to retrieve user-specific settings. However, there is no error handling in case the getCookie function fails or returns an unexpected value. It would be a good idea to add some error handling to these functions to ensure that they fail gracefully if something goes wrong.
Also, the getMyStyles function has a lot of repeated code for setting the fontSize, lineHeight, and backgroundColor properties. This could be simplified by creating a mapping object for each property and using the cookie value as a key to get the corresponding style.
Here's an example of how you could refactor the getMyStyles function:
export const getMyStyles = () => {
const myStyles = JSON.parse(JSON.stringify(defaultStyles));
const fontSize = getCookie("fontSize");
const spacing = getCookie("spacing");
const bg = getCookie("bg");
+ const fontSizeMapping = {
+ large: {
+ heading: "text-4xl",
+ subHeading1: "text-intro",
+ subHeading2: "text-paragraph",
+ para: "text-paragraph",
+ },
+ small: {
+ heading: "text-3xl",
+ subHeading1: "text-2xl",
+ subHeading2: "text-xl",
+ para: "text-md",
+ },
+ };
+
+ const lineHeightMapping = {
+ large: "leading-loose",
+ medium: "leading-normal",
+ small: "leading-none",
+ };
+
+ const backgroundColorMapping = {
+ "bg-dark-bg": "dark-bg",
+ "bg-light-bg": "white",
+ "bg-yellow-bg": "yellow-bg",
+ };
- if (fontSize === "large") {
- myStyles.fontSize = {
- heading: "text-4xl",
- subHeading1: "text-intro",
- subHeading2: "text-paragraph",
- para: "text-paragraph",
- };
- } else if (fontSize == "small") {
- myStyles.fontSize = {
- heading: "text-3xl",
- subHeading1: "text-2xl",
- subHeading2: "text-xl",
- para: "text-md",
- };
- }
-
- if (spacing === "large") {
- myStyles.lineHeight = "leading-loose";
- } else if (spacing === "medium") {
- myStyles.lineHeight = "leading-normal";
- } else if (spacing === "small") {
- myStyles.lineHeight = "leading-none";
- }
-
- if (bg === "bg-dark-bg") {
- myStyles.backgroundColor = "dark-bg";
- } else if (bg === "bg-light-bg") {
- myStyles.backgroundColor = "white";
- } else if (bg === "bg-yellow-bg") {
- myStyles.backgroundColor = "yellow-bg";
- }
+ myStyles.fontSize = fontSizeMapping[fontSize] || myStyles.fontSize;
+ myStyles.lineHeight = lineHeightMapping[spacing] || myStyles.lineHeight;
+ myStyles.backgroundColor = backgroundColorMapping[bg] || myStyles.backgroundColor;
return myStyles;
};
app/shared/functions.ts
Outdated
| export const getMyLanguage = () => { | ||
| const languageId = getCookie("languageId") || 1; | ||
| return ( | ||
| languages.find(({ id }) => String(id) === String(languageId)) || | ||
| languages[0] | ||
| ); | ||
| }; | ||
|
|
||
| export const getMyCommentaryAuthor = () => { | ||
| const commentaryAuthorId = getCookie("commentaryAuthorId") || 16; | ||
| return ( | ||
| commentaryAuthors.find( | ||
| ({ id }) => String(id) === String(commentaryAuthorId) | ||
| ) || commentaryAuthors[0] | ||
| ); | ||
| }; | ||
|
|
||
| export const getMyTranslationAuthor = () => { | ||
| const translationAuthorId = getCookie("translationAuthorId") || 16; | ||
| return ( | ||
| translationAuthors.find( | ||
| ({ id }) => String(id) === String(translationAuthorId) | ||
| ) || translationAuthors[0] | ||
| ); | ||
| }; |
There was a problem hiding this comment.
In the getMyLanguage, getMyCommentaryAuthor, and getMyTranslationAuthor functions, you're using the || operator to provide default values when the getCookie function returns a falsy value. However, this will not work as expected if the cookie value is 0, because 0 is a falsy value in JavaScript. If 0 is a valid ID for a language, commentary author, or translation author, you should use a different method to provide default values. Here's an example of how you could do it:
export const getMyLanguage = () => {
- const languageId = getCookie("languageId") || 1;
+ const languageId = getCookie("languageId") !== null ? getCookie("languageId") : 1;
return (
languages.find(({ id }) => String(id) === String(languageId)) ||
languages[0]
);
};
export const getMyCommentaryAuthor = () => {
- const commentaryAuthorId = getCookie("commentaryAuthorId") || 16;
+ const commentaryAuthorId = getCookie("commentaryAuthorId") !== null ? getCookie("commentaryAuthorId") : 16;
return (
commentaryAuthors.find(
({ id }) => String(id) === String(commentaryAuthorId)
) || commentaryAuthors[0]
);
};
export const getMyTranslationAuthor = () => {
- const translationAuthorId = getCookie("translationAuthorId") || 16;
+ const translationAuthorId = getCookie("translationAuthorId") !== null ? getCookie("translationAuthorId") : 16;
return (
translationAuthors.find(
({ id }) => String(id) === String(translationAuthorId)
) || translationAuthors[0]
);
};
There was a problem hiding this comment.
Files selected (8)
- app/chapter/[chapterNumber]/(chapter)/chapter-page.tsx (3)
- app/chapter/[chapterNumber]/(chapter)/page.tsx (4)
- components/Chapter/VerseList.tsx (1)
- gqty.config.cjs (1)
- lib/getAllChapters.ts (1)
- lib/getChapterData.ts (1)
- lib/getDailyVerse.ts (1)
- src/gqty-client/index.ts (1)
Files ignored due to filter (2)
- package.json
- yarn.lock
Files skipped from review due to trivial changes (3)
- app/chapter/[chapterNumber]/(chapter)/page.tsx
- gqty.config.cjs
- src/gqty-client/index.ts
Review comments generated (11)
- Review: 11
- LGTM: 0
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| type VerseData = Pick< | ||
| GitaVerse, | ||
| "verse_number" | "gita_translations" | "chapter_number" | ||
| >; |
There was a problem hiding this comment.
The change from Omit to Pick in the type definition of VerseData is a significant one. It changes the way VerseData is defined and could potentially impact how it's used throughout the codebase. Ensure that this change doesn't break any existing functionality where VerseData is used, especially if there are properties other than "verse_number", "gita_translations", and "chapter_number" being accessed from VerseData elsewhere in the code.
- type VerseData = Omit<GitaVerse, "word_meanings">;
+ type VerseData = Pick<
+ GitaVerse,
+ "verse_number" | "gita_translations" | "chapter_number"
+ >;
lib/getAllChapters.ts
Outdated
| import { query, resolved } from "src/gqty-client"; | ||
|
|
||
| interface GitaChapters { | ||
| gita_chapters: TChapter[]; | ||
| } | ||
|
|
||
| export async function getAllChapters(): Promise<TChapter[]> { | ||
| const { data }: ApolloQueryResult<GitaChapters> = await apolloClient.query({ | ||
| query: gql` | ||
| query GetAllGitaChapters { | ||
| gita_chapters { | ||
| id | ||
| chapter_number | ||
| chapter_summary | ||
| name_translated | ||
| verses_count | ||
| } | ||
| } | ||
| `, | ||
| export const getAllChapters = () => | ||
| resolved(() => { | ||
| const gitaChapters = query.gita_chapters(); | ||
| return gitaChapters.map((gitaChapter) => ({ | ||
| id: gitaChapter.id!, | ||
| chapter_number: gitaChapter.chapter_number!, | ||
| chapter_summary: gitaChapter.chapter_summary!, | ||
| name_translated: gitaChapter.name_translated!, | ||
| verses_count: gitaChapter.verses_count!, | ||
| })); | ||
| }); |
There was a problem hiding this comment.
The new implementation of getAllChapters function seems to be using the gqty-client for querying data instead of the previous apolloClient. However, there is a potential issue with this approach. The use of non-null assertions (!) in lines 7-11 can lead to runtime errors if any of these fields are null or undefined. It would be safer to handle potential null values gracefully.
- id: gitaChapter.id!,
- chapter_number: gitaChapter.chapter_number!,
- chapter_summary: gitaChapter.chapter_summary!,
- name_translated: gitaChapter.name_translated!,
- verses_count: gitaChapter.verses_count!,
+ id: gitaChapter.id || '',
+ chapter_number: gitaChapter.chapter_number || 0,
+ chapter_summary: gitaChapter.chapter_summary || '',
+ name_translated: gitaChapter.name_translated || '',
+ verses_count: gitaChapter.verses_count || 0,
lib/getChapterData.ts
Outdated
| import { order_by, query, resolved } from "src/gqty-client"; | ||
|
|
||
| interface GitaChaptersNumber { | ||
| gita_chapters: { chapter_number: number }[]; | ||
| } | ||
| export const getChapterNumbers = () => | ||
| resolved(() => ({ | ||
| gita_chapters: query | ||
| .gita_chapters() | ||
| .map(({ chapter_number }) => ({ chapter_number: chapter_number! })), | ||
| })); |
There was a problem hiding this comment.
The getChapterNumbers function has been refactored to use the gqty-client instead of Apollo. The new implementation looks good and should work as expected.
lib/getChapterData.ts
Outdated
| export const getChapterData = (chapterNumber: number) => | ||
| resolved(() => { | ||
| const gitaChapter = query.gita_chapters_by_pk({ | ||
| id: chapterNumber, | ||
| }); | ||
|
|
||
| return data; | ||
| } | ||
| const gitaVerses = query.gita_verses({ | ||
| where: { | ||
| chapter_id: { | ||
| _eq: chapterNumber, | ||
| }, | ||
| }, | ||
| order_by: [ | ||
| { | ||
| verse_number: order_by.asc, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| export async function getChapterData( | ||
| chapterNumber: string | ||
| ): Promise<GitaChapter> { | ||
| const { data }: ApolloQueryResult<GitaChapter> = await apolloClient.query({ | ||
| query: gql` | ||
| query MyQuery { | ||
| gita_chapters_by_pk(id: ${chapterNumber ?? 1}) { | ||
| chapter_number | ||
| chapter_summary | ||
| name_translated | ||
| verses_count | ||
| } | ||
| gita_verses(where: {chapter_id: {_eq: ${ | ||
| chapterNumber ?? 1 | ||
| }}}, order_by: {verse_number: asc}) { | ||
| id | ||
| verse_number | ||
| word_meanings | ||
| chapter_number | ||
| gita_translations(where: {language: {_eq: "english"}, author_name: {_eq: "Swami Gambirananda"}}) { | ||
| description | ||
| } | ||
| } | ||
| } | ||
| `, | ||
| }); | ||
| if (gitaChapter === null) { | ||
| return null; | ||
| } | ||
|
|
||
| return data; | ||
| } | ||
| return { | ||
| gita_chapters_by_pk: { | ||
| chapter_number: gitaChapter.chapter_number!, | ||
| chapter_summary: gitaChapter.chapter_summary!, | ||
| name_translated: gitaChapter.name_translated!, | ||
| verses_count: gitaChapter.verses_count!, | ||
| }, | ||
| gita_verses: gitaVerses.map((gitaVerse) => ({ | ||
| id: gitaVerse.id!, | ||
| verse_number: gitaVerse.verse_number!, | ||
| word_meanings: gitaVerse.word_meanings!, | ||
| chapter_number: gitaVerse.chapter_number!, | ||
| gita_translations: gitaVerse | ||
| .gita_translations({ | ||
| where: { | ||
| language: { _eq: "english" }, | ||
| author_name: { _eq: "Swami Gambirananda" }, | ||
| }, | ||
| }) | ||
| .map((gitaTranslation) => ({ | ||
| description: gitaTranslation.description!, | ||
| })), | ||
| })), | ||
| }; | ||
| }); |
There was a problem hiding this comment.
The getChapterData function has also been refactored to use the gqty-client. However, there's a potential issue with the null check for gitaChapter. If gitaChapter is null, the function returns null immediately, which might not be the expected behavior if there are verses associated with the chapter number. Consider handling this scenario more gracefully.
if (gitaChapter === null) {
return null;
}Also, the getChapterData function now expects a number for chapterNumber, whereas it previously expected a string. This could potentially cause issues if other parts of the codebase are still passing a string. Ensure that all calls to getChapterData have been updated to pass a number.
Finally, the gita_translations query is hard-coded to only retrieve English translations by Swami Gambirananda. If this is intentional and fits your application's requirements, then it's fine. However, if you want to make it more flexible, consider passing the language and author as parameters to the getChapterData function.
where: {
language: { _eq: "english" },
author_name: { _eq: "Swami Gambirananda" },
},
lib/getDailyVerse.ts
Outdated
| import { order_by, query, resolved } from "src/gqty-client"; | ||
|
|
||
| export const getDailyVerse = async () => { | ||
| const { | ||
| data: { verse_of_the_day }, | ||
| }: ApolloQueryResult<VersesOfTheDay> = await apolloClient.query({ | ||
| query: gql` | ||
| query MyQuery { | ||
| verse_of_the_day(order_by: { date: desc }) { | ||
| date | ||
| id | ||
| verse_order | ||
| } | ||
| } | ||
| `, | ||
| }); | ||
| const whereGitaAuthor = { | ||
| where: { | ||
| gita_author: { | ||
| name: { | ||
| _eq: "Swami Sivananda", | ||
| }, | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The whereGitaAuthor object is defined but not used in the getDailyVerse function. It's better to define it inside the function where it's used, to avoid unnecessary global scope pollution.
- const whereGitaAuthor = {
- where: {
- gita_author: {
- name: {
- _eq: "Swami Sivananda",
- },
- },
- },
- };
lib/getDailyVerse.ts
Outdated
| if (!gitaVerse) { | ||
| throw new Error("Gita verse of the day not found"); | ||
| } |
There was a problem hiding this comment.
Throwing a generic Error might not be very helpful for debugging if the Gita verse of the day is not found. Consider creating a custom error class that can provide more context about the error.
- throw new Error("Gita verse of the day not found");
+ throw new VerseNotFoundError("Gita verse of the day not found");In this case, VerseNotFoundError would be a custom error class that you would need to define.
lib/getDailyVerse.ts
Outdated
| return { | ||
| id: gitaVerse.id!, | ||
| chapter_number: gitaVerse.chapter_number!, | ||
| verse_number: gitaVerse.verse_number!, | ||
| text: gitaVerse.text!, | ||
| transliteration: gitaVerse.transliteration!, | ||
| word_meanings: gitaVerse.word_meanings!, | ||
| gita_translations: gitaVerse | ||
| .gita_translations(whereGitaAuthor) | ||
| .map((gitaTranslation) => ({ | ||
| description: gitaTranslation.description!, | ||
| })), | ||
| gita_commentaries: gitaVerse | ||
| .gita_commentaries(whereGitaAuthor) | ||
| .map((gitaTranslation) => ({ | ||
| description: gitaTranslation.description!, | ||
| })), | ||
| gita_chapter: { | ||
| verses_count: gitaVerse.gita_chapter?.verses_count || 0, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The returned object has many fields with non-null assertion (!). This could potentially lead to runtime errors if any of these fields are null or undefined. Consider adding checks for these fields before accessing them or handle potential null or undefined values gracefully.
- id: gitaVerse.id!,
+ id: gitaVerse.id || 'default_id',This is just an example. You should replace 'default_id' with a suitable default value or handle the situation appropriately based on your application's requirements.
| import { SvgChapterBackground } from "components/svgs"; | ||
| import PageNavigator from "components/Chapter/PageNavigator"; | ||
| import classNames from "utils/classNames"; | ||
| import VerseNavigator from "components/Chapter/VerseNavigator"; | ||
| import VerseList from "components/Chapter/VerseList"; | ||
| import useMyStyles from "hooks/useMyStyles"; | ||
|
|
||
| interface Props { | ||
| chapterData: GitaChapterData; | ||
| versesData: GitaVerse[]; | ||
| versesData: Pick< | ||
| GitaVerse, | ||
| "id" | "verse_number" | "gita_translations" | "chapter_number" | ||
| >[]; |
There was a problem hiding this comment.
The import statements have been updated to use absolute paths instead of relative paths. This is a good practice as it makes the code more readable and maintainable, especially in larger projects where files might be deeply nested.
- import { SvgChapterBackground } from "../../../../components/svgs";
- import PageNavigator from "../../../../components/Chapter/PageNavigator";
- import classNames from "../../../../utils/classNames";
- import VerseNavigator from "../../../../components/Chapter/VerseNavigator";
- import VerseList from "../../../../components/Chapter/VerseList";
- import useMyStyles from "../../../../hooks/useMyStyles";
+ import { SvgChapterBackground } from "components/svgs";
+ import PageNavigator from "components/Chapter/PageNavigator";
+ import classNames from "utils/classNames";
+ import VerseNavigator from "components/Chapter/VerseNavigator";
+ import VerseList from "components/Chapter/VerseList";
+ import useMyStyles from "hooks/useMyStyles";| @@ -29,7 +32,7 @@ | |||
| }: Props) { | |||
| const [viewNavigation, setViewNavigation] = useState(false); | |||
| const [verseId, setVerseId] = useState(0); | |||
| const [isAscSorted, setisAscSorted] = useState(true); | |||
| const [isAscSorted, setIsAscSorted] = useState(true); | |||
There was a problem hiding this comment.
The state variable isAscSorted has been renamed from setisAscSorted for better readability. It's a good practice to follow camelCase naming convention for variables in JavaScript.
- const [isAscSorted, setisAscSorted] = useState(true);
+ const [isAscSorted, setIsAscSorted] = useState(true);| @@ -118,7 +121,7 @@ | |||
| <button | |||
| type="button" | |||
| className="-ml-px relative inline-flex items-center space-x-2 px-4 py-2 border border-gray-300 text-sm font-medium dark:bg-dark-100 rounded-r-md text-gray-700 dark:text-gray-50 bg-gray-50 hover:bg-gray-100 dark:hover:bg-dark-bg focus:outline-none focus:ring-1 focus:ring-my-orange focus:border-my-orange" | |||
| onClick={() => setisAscSorted(!isAscSorted)} | |||
| onClick={() => setIsAscSorted(!isAscSorted)} | |||
There was a problem hiding this comment.
The function name setIsAscSorted is used correctly here. The previous version had a typo in the function name (setisAscSorted). Good job on fixing this.
- onClick={() => setisAscSorted(!isAscSorted)}
+ onClick={() => setIsAscSorted(!isAscSorted)}
There was a problem hiding this comment.
Files selected (6)
- app/chapter/[chapterNumber]/verse/[verseNumber]/page.tsx (2)
- app/verse-of-the-day/page.tsx (2)
- lib/apolloClient.ts (1)
- lib/getDailyVerse.ts (1)
- lib/getVerseData.ts (1)
- lib/subscribeUser.ts (1)
Files ignored due to filter (2)
- package.json
- yarn.lock
Files skipped from review due to trivial changes (3)
- lib/getDailyVerse.ts
- lib/getVerseData.ts
- lib/subscribeUser.ts
Review comments generated (6)
- Review: 5
- LGTM: 1
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| export async function generateMetadata(): Promise<Metadata> { | ||
| const { chapter_number, verse_number } = await getDailyVerse(); | ||
| const gitaVerse = await getDailyVerse(); | ||
|
|
||
| if (!gitaVerse) { | ||
| return {}; | ||
| } | ||
|
|
||
| const { chapter_number, verse_number } = gitaVerse; |
There was a problem hiding this comment.
The generateMetadata function now handles the case where getDailyVerse might return null or undefined. This is a good improvement for error handling. However, it would be better to provide some default metadata instead of an empty object when gitaVerse is not available. This will ensure that your page always has some metadata, which is important for SEO.
if (!gitaVerse) {
return {};
+ return {
+ title: "Bhagavad Gita - BhagavadGita.io",
+ // other default metadata properties...
+ };
}
app/verse-of-the-day/page.tsx
Outdated
| if (!dailyVerse) { | ||
| return <h1>Not found</h1>; | ||
| } |
There was a problem hiding this comment.
The check for dailyVerse being null or undefined before rendering the VerseOfTheDay component is a good practice. However, returning a simple <h1>Not found</h1> might not be the best user experience. Consider returning a more descriptive message or a custom error component.
return <h1>Not found</h1>;
+ return <ErrorComponent message="Daily verse not found" />;| import { headers } from "next/headers"; | ||
|
|
||
| import { Metadata } from "next"; | ||
| import { getVerseId } from "../../../../../lib/getVerseData"; | ||
| import { getVerseData } from "lib/getVerseData"; | ||
| import VersePage from "./verse-page"; | ||
| import { getLanguageSettings } from "app/shared/functions"; | ||
|
|
||
| function VerseFallback() { | ||
| return <>Loading...</>; | ||
| } | ||
| export const dynamic = "force-dynamic"; | ||
|
|
||
| type Props = { | ||
| params: { chapterNumber: string; verseNumber: string }; | ||
| }; | ||
|
|
||
| export async function generateStaticParams() { | ||
| const data = await getVerseId(); | ||
| // export async function generateStaticParams() { | ||
| // const data = await getVerseId(); | ||
|
|
||
| return data.gita_verses.map(({ chapter_number, verse_number }) => ({ | ||
| params: { | ||
| chapterNumber: chapter_number.toString(), | ||
| verseNumber: verse_number.toString(), | ||
| }, | ||
| })); | ||
| } | ||
| // return data.gita_verses.map(({ chapter_number, verse_number }) => ({ | ||
| // params: { | ||
| // chapterNumber: chapter_number.toString(), | ||
| // verseNumber: verse_number.toString(), | ||
| // }, | ||
| // })); | ||
| // } |
There was a problem hiding this comment.
The commented out function generateStaticParams was previously used to generate static parameters for each verse. If this function is no longer needed, it's better to remove the commented code to maintain cleanliness and readability of the codebase.
- // export async function generateStaticParams() {
- // const data = await getVerseId();
-
- // return data.gita_verses.map(({ chapter_number, verse_number }) => ({
- // params: {
- // chapterNumber: chapter_number.toString(),
- // verseNumber: verse_number.toString(),
- // },
- // }));
- // }| const headersList = headers(); | ||
| const languageSettings = getLanguageSettings({ | ||
| languageId: parseInt(headersList.get("x-settings-l") || ""), | ||
| translationAuthorId: parseInt(headersList.get("x-settings-t") || ""), | ||
| commentaryAuthorId: parseInt(headersList.get("x-settings-c") || ""), | ||
| }); | ||
|
|
||
| const verseData = await getVerseData( | ||
| chapterNumber, | ||
| verseNumber, | ||
| languageSettings.commentaryAuthor.name, | ||
| languageSettings.translationAuthor.name | ||
| ); |
There was a problem hiding this comment.
The usage of headers() function directly might not be safe as it can potentially return null or undefined. It would be better to add error handling here to prevent potential runtime errors. Also, the parsing of headers into integers should be handled with care. If the header does not exist or its value is not a valid integer, parseInt will return NaN, which could lead to unexpected behavior in subsequent code. Consider adding checks to ensure that the parsed values are valid integers before using them.
- const headersList = headers();
+ const headersList = headers() || new Headers();
const languageSettings = getLanguageSettings({
- languageId: parseInt(headersList.get("x-settings-l") || ""),
- translationAuthorId: parseInt(headersList.get("x-settings-t") || ""),
- commentaryAuthorId: parseInt(headersList.get("x-settings-c") || ""),
+ languageId: parseInt(headersList.get("x-settings-l")) || undefined,
+ translationAuthorId: parseInt(headersList.get("x-settings-t")) || undefined,
+ commentaryAuthorId: parseInt(headersList.get("x-settings-c")) || undefined,
});| if (!verseData) { | ||
| return <h1>Not found</h1>; | ||
| } |
There was a problem hiding this comment.
Returning an <h1> tag when verseData is not found may not provide a good user experience. Consider returning a more descriptive error message or a custom error component.
- if (!verseData) {
- return <h1>Not found</h1>;
- }
+ if (!verseData) {
+ return <ErrorComponent message="Verse data not found" />;
+ }
There was a problem hiding this comment.
Files selected (1)
- lib/getDailyVerse.ts (1)
Files skipped from review due to trivial changes (1)
- lib/getDailyVerse.ts
Review comments generated (0)
- Review: 0
- LGTM: 0
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
Summary by CodeRabbit
handleSubscribefunction and removed unused components and variables.BackgroundImagecomponent and modifying SVG elements in theDarkModeTogglecomponent..gitignorefile to ignore more files and directories.