Add chapter navigation and table of contents#233
Conversation
|
@DivyaShreeS09 is attempting to deploy a commit to the Adarsh's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@DivyaShreeS09 ci build fails here too |
|
The CI build appears to be failing in authentication-related files such as "src/app/(auth)/login/page.tsx" and "src/app/(auth)/layout.tsx". The lint check passes successfully for this PR, and these files are not part of my changes for Issue #90 (chapter navigation and table of contents). Could you please confirm whether this failure is related to a recent change in the base branch or if any additional action is required from my side? Thank you. |
MRIARC-08
left a comment
There was a problem hiding this comment.
Thanks for the chapter navigation work. This needs a rebase before it can merge. Current main now renders chapter markdown through MarkdownViewer, but this PR changes ChapterContent back to MarkdownRenderer and adds heading IDs there. That would undo the newer renderer path merged earlier.
There is also a hook-order issue in ChapterTOC: it returns early on !content before calling useEffect, so the hook order can change between renders. Please rebase on latest main, integrate the TOC with MarkdownViewer, and move hooks before conditional returns.
0d37aaa to
7cc3fc8
Compare
|
@MRIARC-08 Kindly review my PR updates |
MRIARC-08
left a comment
There was a problem hiding this comment.
@DivyaShreeS09 The latest branch still has an early return in the chapter page, leaving the new ChapterContent call with previous/next data unreachable, so the navigation feature is not actually rendered. It also continues to modify the legacy MarkdownRenderer path rather than the current reader path. Please remove the unreachable duplicate return and integrate the TOC/heading ids with the renderer used by the live chapter page.
MRIARC-08
left a comment
There was a problem hiding this comment.
@DivyaShreeS09 I retested the exact merge result against current main. It currently fails the production build: ChapterContent has malformed JSX near the Print button around line 144. The chapter page also still returns before the new previous/next ChapterContent render, so that navigation remains unreachable, and the TOC heading ids are still implemented in the unused MarkdownRenderer while the live page renders MarkdownViewer. Please fix those three points and rerun build/lint.
6264c97 to
b50ce40
Compare
|
@MRIARC-08 |
MRIARC-08
left a comment
There was a problem hiding this comment.
@DivyaShreeS09 This branch now conflicts with current main. After rebasing, please make the table of contents use the same duplicate-safe heading IDs as the active MarkdownViewer; changing the unused MarkdownRenderer does not fix the live chapter page. Also preserve the recently merged resume-reading progress behavior in ChapterContent.
ee0c238 to
61960ef
Compare
|
@MRIARC-08 Kindly review my PR updates |
MRIARC-08
left a comment
There was a problem hiding this comment.
@DivyaShreeS09 The remaining navigation issues are in the live path. The TOC IDs are still duplicate-prone, and the duplicate-ID handling was added to MarkdownRenderer while the chapter page uses MarkdownViewer. The observer also will not refresh when content changes, and first/last chapter state can remain stale. Please use one duplicate-safe slugger for the TOC and active viewer, rerun observer setup when content changes, reset navigation state on route changes, and remove the placeholder Start Quiz/related-topic actions.
|
@MRIARC-08 Addressed the review comments:
|
MRIARC-08
left a comment
There was a problem hiding this comment.
@DivyaShreeS09 The shared duplicate-safe slugger, active viewer integration, observer refresh, navigation reset, and placeholder removal are now addressed. The merge result still regresses the audited lockfile by removing the security overrides and downgrading several packages; regenerate pnpm-lock.yaml from current main with only github-slugger added. Please also restore the existing “content not yet available” fallback and run the formatter: focused lint currently reports 46 errors.
|
@MRIARC-08 I've addressed the requested changes, merged the latest main branch, resolved the MarkdownRenderer conflict, removed the lint issues, regenerated the lockfile with github-slugger, and verified the project builds successfully. |
|
@DivyaShreeS09 I rechecked this after the latest merges. The PR now has a merge conflict in |
|
Hi @MRIARC-08, |
MRIARC-08
left a comment
There was a problem hiding this comment.
@DivyaShreeS09 I rechecked this against the current main. The earlier src/app/page.js/src/app/page.tsx build issue should be resolved now because main has been restored to include src/app/page.tsx again, and your branch merges cleanly locally.
The remaining blockers are still in the PR itself:
-
pnpm-lock.yamlremoves the securityoverridesblock from current main while addinggithub-slugger. Please regenerate/update the lockfile from currentmainwithout dropping those overrides. -
Focused lint still reports 18 Prettier errors in the touched chapter/TOC files.
-
The existing
@next/next/no-img-elementwarning inMarkdownVieweris unrelated and not a blocker.Please restore the lockfile overrides and run the formatter on the touched files.
|
Addressed the review feedback:
|
MRIARC-08
left a comment
There was a problem hiding this comment.
@DivyaShreeS09 This is much closer: the lockfile no longer removes the security overrides, and focused ESLint has no errors.
Local checks:
- Focused ESLint on the touched chapter/markdown files passes with only the existing Markdown image warning.
pnpm testpasses: 8 files / 35 tests.pnpm exec tsc --noEmitcurrently fails in my existing install becausegithub-sluggeris newly added and not installed in localnode_modules; please make sure a freshpnpm install --frozen-lockfileplus typecheck passes.
Remaining functional blocker: the chapter page still fetches the current chapter with only:
/api/ncert/chapter?chapter=${params.chapter}
Current main requires the parent class and subject context for this endpoint. Please add class/subject or classId/subjectId to that request, and also guard chaptersRes.message.chapters before reading it so a failed chapters response does not crash the page.
Since this is a visible chapter navigation/TOC UI change, it will still need manual UI review before merge.
|
@DivyaShreeS09 I rechecked the latest commit. It now merges cleanly, focused ESLint is clean apart from the existing Next |
Summary
Implemented chapter navigation and content discovery improvements for NCERT chapter pages.
Fixes #90
Features Added
Changes
New Components
src/components/ChapterTOC.tsxsrc/components/ChapterBreadcrumb.tsxsrc/components/ChapterNavigation.tsxUpdated Files
src/app/ncert/[class]/[subject]/[chapter]/page.tsxsrc/components/ChapterContent.tsxsrc/components/MarkdownRenderer.tsxTesting
Verified successfully with:
Issues Encountered
TypeScript Issues
anytype lint violationsResolved by:
anywith typed interfacesFormatting & Linting
Resolved by:
Files Added
Files Modified