Resolve "#31-Transmission chain links"#61
Conversation
AbdulrhmnGhanem
had a problem deploying
to
preview
GitHub Actions
Failure
WalkthroughAdds narrator-to-narrator pages and DB APIs; replaces hadith export with a Poe-managed search-index generator and switches search UI to consume Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant NG as NetworkGraph (client)
participant R as Next Router
participant P as Narrator→Narrator Page
participant DB as lib/sqlite
U->>NG: Click edge (source → target)
NG->>R: router.push("/narrator/{source}/to/{target}")
R->>P: Load page with params
P->>DB: getHadithsFromNarratorToNarrator(from, to, "Sahih Bukhari", limit)
DB-->>P: Hadith list
alt hadiths found
P-->>U: Render HadithList + breadcrumbs
else none
P-->>U: Render RTL NotFound page
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
AbdulrhmnGhanem
temporarily deployed
to
preview
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (3)
components/hadith-card.tsx (2)
8-20: useRouter requires a Client Component — add "use client".This file uses
useRouterbut lacks the"use client"directive, which will throw at runtime in Next.js App Router.Apply this diff at the top:
+ "use client"; + import { Card } from "@/components/ui/card"; import { arabicTexts, toArabicNumerals } from "@/lib/arabic-utils"; import { getArabicSource } from "@/lib/book-mapping"; import { LAYOUT } from "@/lib/layout-constants"; import { HadithWithFirstNarrator } from "@/lib/sqlite"; import { cleanName } from "@/lib/utils"; -import Link from "next/link"; import { useRouter } from "next/navigation";
21-27: Avoid nesting interactive elements (button inside anchor) and ensure chapter is encoded.Placing
<button>inside<Link>renders<button>inside<a>, which is invalid HTML and harms accessibility. Also, the outer navigation didn’t URL-encodechapter. Switch to programmatic navigation on the container and drop the outer Link.Apply:
- return ( - <Link - href={`/hadith/${encodeURIComponent(hadith.source)}/${hadith.chapter}/${hadith.hadith_no}`} - hrefLang="ar" - className={className} - > + return ( + <div + role="link" + tabIndex={0} + onClick={() => + router.push( + `/hadith/${encodeURIComponent(hadith.source)}/${encodeURIComponent(hadith.chapter)}/${hadith.hadith_no}`, + ) + } + onKeyDown={(e) => { + if (e.key === "Enter") { + router.push( + `/hadith/${encodeURIComponent(hadith.source)}/${encodeURIComponent(hadith.chapter)}/${hadith.hadith_no}`, + ); + } + }} + className={className} + aria-label="عرض الحديث" + > @@ - </Link> + </div>Note: Keeping
e.stopPropagation()on inner buttons remains valid to prevent the parent click handler from firing.Also applies to: 80-80
app/search/page.tsx (1)
55-66: Prevent infinite spinner and add robust error handling for search index fetch.If
/search_index.jsonfails or returns non-200,loading/initializingremain true and the page spins indefinitely. Add ok-check, error handling, and cleanup to avoid state updates after unmount.Apply this diff within the effect to handle errors and abort on unmount:
useEffect(() => { setLoading(true); setInitializing(true); - const basePath = process.env.NEXT_PUBLIC_BASE_PATH || ""; - fetch(`${basePath}/search_index.json`) - .then((res) => res.json()) - .then((data) => { - setAllHadiths(data); - setLoading(false); - setInitializing(false); - }); + const basePath = process.env.NEXT_PUBLIC_BASE_PATH || ""; + const controller = new AbortController(); + const { signal } = controller; + fetch(`${basePath}/search_index.json`, { signal }) + .then((res) => { + if (!res.ok) throw new Error(`Failed to load search index: ${res.status}`); + return res.json(); + }) + .then((data: HadithWithFirstNarrator[]) => { + setAllHadiths(data); + }) + .catch((err) => { + // swallow aborts, log other errors + if ((err as any)?.name !== "AbortError") { + console.error("Failed to load search index", err); + setAllHadiths([]); + } + }) + .finally(() => { + // guard against updates after abort/unmount + if (!controller.signal.aborted) { + setLoading(false); + setInitializing(false); + } + }); + return () => controller.abort(); }, []);
🧹 Nitpick comments (14)
components/hadith-card.tsx (1)
67-71: Replace the magic number with the layout constant for consistency.Use the defined
LAYOUT.HEADER_SPACINGinstead of hard-coding140.You can compute this via CSS calc to avoid manual parsing:
- maxHeight: `${LAYOUT.HADITH_CARD_HEIGHT - 140}px`, + maxHeight: `calc(${LAYOUT.HADITH_CARD_HEIGHT}px - ${LAYOUT.HEADER_SPACING})`,lib/types/graph-config.ts (1)
2-2: Use type-only import for cleaner builds and clearer intent.-import { GraphConfiguration, GraphProps } from "react-d3-graph"; +import type { GraphConfiguration, GraphProps } from "react-d3-graph";app/narrator/[name]/to/[receiver]/not-found.tsx (1)
5-6: Add dir="rtl" on the container for proper bidi/layout defaults.You’re already setting right-aligned text and RTL spacing; setting
dir="rtl"ensures punctuation and embedded LTR segments render correctly by default.- <div className="container mx-auto px-4 py-8 text-center"> + <div dir="rtl" className="container mx-auto px-4 py-8 text-center">components/predecessors-successors-chain.tsx (1)
535-537: Good micro-optimization with nodeMap.Precomputing a Map for O(1) lookups simplifies the click handler. If you see re-renders with stable data, consider
useMemoto avoid rebuilding.const nodeMap = useMemo( () => new Map(graphData.nodes.map((n) => [n.id, n] as const)), [graphData.nodes], );scripts/gen_search_index.py (4)
28-31: Close DB resources to avoid descriptor leaks in CI or long-lived runsThe connection and cursor are not closed. Even though this is a short-lived script, explicit cleanup is safer and avoids file-descriptor leaks in pipelines.
Apply this diff to cleanly close resources:
with open(OUT_PATH, "w", encoding="utf-8") as f: json.dump(all_hadiths, f, ensure_ascii=False, indent=2) -print(f"Exported {len(all_hadiths)} hadiths to {OUT_PATH}") + # Cleanly close DB resources + cursor.close() + conn.close() +print(f"Exported {len(all_hadiths)} hadiths to {OUT_PATH}")Also applies to: 55-57
43-43: Add strict=True to zip() to catch schema/row-length mismatches (ruff B905)This guards against silent truncation if the schema changes and helps satisfy the linter.
-all_hadiths = [dict(zip(columns, row)) for row in rows] +all_hadiths = [dict(zip(columns, row, strict=True)) for row in rows]
29-31: Open the database in read-only mode to prevent accidental writes/locksAs this script only reads, using SQLite URI with mode=ro is safer and can reduce lock contention.
-conn = sqlite3.connect(DB_PATH) +conn = sqlite3.connect(f"file:{DB_PATH.as_posix()}?mode=ro", uri=True) cursor = conn.cursor()
44-46: Remove stale commentThis comment refers to an in-place step that no longer exists. Cleaning it up avoids confusion.
-# Strip diacritics from text_ar in-place -lib/sqlite.ts (2)
243-258: Keep narrator_name semantics consistent (first narrator in chain)Elsewhere, narrator_name represents the first narrator (position = 1). Here it’s set to the “from” narrator (which can be any position). This may produce inconsistent UI behavior.
Suggested SQL change to always attach the first narrator as narrator_name:
- statements.getHadithsFromNarratorToNarrator = db.prepare(` - SELECT h.*, r1.name as narrator_name + statements.getHadithsFromNarratorToNarrator = db.prepare(` + SELECT h.*, r_first.name as narrator_name FROM hadiths h JOIN hadith_chains c1 ON h.source = c1.source AND h.chapter_no = c1.chapter_no AND h.hadith_no = c1.hadith_no JOIN hadith_chains c2 ON h.source = c2.source AND h.chapter_no = c2.chapter_no AND h.hadith_no = c2.hadith_no AND c2.position = c1.position + 1 + JOIN hadith_chains c_first ON h.source = c_first.source + AND h.chapter_no = c_first.chapter_no + AND h.hadith_no = c_first.hadith_no + AND c_first.position = 1 JOIN rawis r1 ON c1.scholar_indx = r1.scholar_indx JOIN rawis r2 ON c2.scholar_indx = r2.scholar_indx + JOIN rawis r_first ON c_first.scholar_indx = r_first.scholar_indx WHERE r1.name = $from_narrator AND r2.name = $to_narrator ORDER BY h.source, h.chapter_no, h.hadith_no LIMIT $limit `);If the UI intentionally wants the “from” narrator to appear in narrator_name, please confirm so we can document that divergence from the type’s implied meaning and adjust tests accordingly.
260-273: Simplify query: DISTINCT with GROUP BY and HAVING COUNT(*) > 0 are redundantCOUNT(*) after an inner join is always ≥ 1, and GROUP BY already yields one row per pair; DISTINCT is unnecessary.
- statements.getNarratorPairs = db.prepare(` - SELECT DISTINCT r1.name as from_narrator, r2.name as to_narrator, COUNT(*) as hadith_count + statements.getNarratorPairs = db.prepare(` + SELECT r1.name as from_narrator, r2.name as to_narrator, COUNT(*) as hadith_count FROM hadith_chains c1 JOIN hadith_chains c2 ON c1.source = c2.source AND c1.chapter_no = c2.chapter_no AND c1.hadith_no = c2.hadith_no AND c2.position = c1.position + 1 JOIN rawis r1 ON c1.scholar_indx = r1.scholar_indx JOIN rawis r2 ON c2.scholar_indx = r2.scholar_indx GROUP BY r1.name, r2.name - HAVING COUNT(*) > 0 ORDER BY hadith_count DESC `);tests/sqlite.test.ts (2)
141-170: Stabilize test by using an actual existing pair instead of the first two narratorsThe first two narrators may not be consecutive in any chain; using getNarratorPairs ensures we test a real edge.
- test("getHadithsFromNarratorToNarrator returns hadiths narrated from one narrator to another", () => { - // First get some narrators to test with - const narrators = getNarrators(); - expect(narrators.length).toBeGreaterThan(1); - - // Use first two narrators for testing - const fromNarrator = narrators[0].name; - const toNarrator = narrators[1].name; + test("getHadithsFromNarratorToNarrator returns hadiths narrated from one narrator to another", () => { + // Use a real consecutive pair to avoid false negatives + const pairs = getNarratorPairs(); + expect(pairs.length).toBeGreaterThan(0); + const { from_narrator: fromNarrator, to_narrator: toNarrator } = pairs[0]; const hadiths = getHadithsFromNarratorToNarrator( fromNarrator, toNarrator, 10, ); expect(hadiths).toBeArray(); // Each hadith should have all required properties for HadithWithFirstNarrator interface hadiths.forEach((hadith) => { expect(hadith).toHaveProperty("id"); expect(hadith).toHaveProperty("hadith_id"); expect(hadith).toHaveProperty("source"); expect(hadith).toHaveProperty("chapter_no"); expect(hadith).toHaveProperty("hadith_no"); expect(hadith).toHaveProperty("chapter"); expect(hadith).toHaveProperty("text_ar"); expect(hadith).toHaveProperty("text_en"); expect(hadith).toHaveProperty("explanation"); expect(hadith).toHaveProperty("narrator_name"); }); });
188-191: Remove duplicate database close() callThere’s already an afterAll block closing the DB in the first describe. The second one is redundant.
- afterAll(() => { - close(); - });app/narrator/[name]/to/[receiver]/page.tsx (2)
19-27: Use conventional params typing (not Promise) and avoid unnecessary awaitNext.js App Router passes params as a plain object. Awaiting it works at runtime but is non-idiomatic and obscures intent.
-export async function generateMetadata({ - params, -}: { - params: Promise<{ name: string; receiver: string }>; -}): Promise<Metadata> { - const { name, receiver } = await params; +export async function generateMetadata({ + params, +}: { + params: { name: string; receiver: string }; +}): Promise<Metadata> { + const { name, receiver } = params;-export default async function NarratorToNarratorPage({ - params, -}: { - params: Promise<{ name: string; receiver: string }>; -}) { - const { name, receiver } = await params; +export default async function NarratorToNarratorPage({ + params, +}: { + params: { name: string; receiver: string }; +}) { + const { name, receiver } = params;Also applies to: 39-47
10-17: Potential SSG explosion: pre-rendering all narrator pairsPre-generating a page per pair can scale poorly and slow builds. Consider capping to top-N pairs, introducing ISR (revalidate), or using on-demand generation.
Examples:
- Limit generateStaticParams to, e.g., the top 5–10k pairs by hadith_count.
- Use dynamicParams: true and return a 404 for rarely requested pairs, or implement fallback rendering.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
.github/workflows/deploy.yml(1 hunks).gitignore(1 hunks)app/narrator/[name]/to/[receiver]/not-found.tsx(1 hunks)app/narrator/[name]/to/[receiver]/page.tsx(1 hunks)app/search/page.tsx(1 hunks)components/hadith-card.tsx(3 hunks)components/predecessors-successors-chain.tsx(4 hunks)lib/sqlite.ts(4 hunks)lib/types/graph-config.ts(2 hunks)pyproject.toml(1 hunks)scripts/export_hadiths_json.py(0 hunks)scripts/gen_search_index.py(1 hunks)tests/sqlite.test.ts(2 hunks)
💤 Files with no reviewable changes (1)
- scripts/export_hadiths_json.py
🧰 Additional context used
🧬 Code Graph Analysis (7)
app/narrator/[name]/to/[receiver]/not-found.tsx (4)
app/narrator/[name]/not-found.tsx (1)
NotFound(1-12)app/hadith/[source]/[chapter]/not-found.tsx (1)
NotFound(1-12)app/narrator/page.tsx (1)
NarratorsPage(32-49)app/narrator/[name]/page.tsx (1)
NarratorPage(201-265)
components/predecessors-successors-chain.tsx (1)
lib/types/graph.ts (2)
NarratorGraphNode(15-15)GraphLink(9-13)
app/narrator/[name]/to/[receiver]/page.tsx (3)
lib/sqlite.ts (2)
getNarratorPairs(450-453)getHadithsFromNarratorToNarrator(437-448)components/hadith-list.tsx (1)
HadithList(12-47)app/narrator/[name]/page.tsx (2)
generateStaticParams(194-199)generateMetadata(24-86)
tests/sqlite.test.ts (1)
lib/sqlite.ts (3)
getNarrators(283-286)getHadithsFromNarratorToNarrator(437-448)getNarratorPairs(450-453)
scripts/gen_search_index.py (1)
scripts/export_hadiths_json.py (2)
strip_diacritics(16-25)normalize_whitespace(10-13)
components/hadith-card.tsx (3)
lib/book-mapping.ts (1)
getArabicSource(10-12)lib/utils.ts (1)
cleanName(8-17)lib/layout-constants.ts (1)
LAYOUT(2-15)
lib/types/graph-config.ts (1)
lib/types/graph.ts (4)
GraphLink(9-13)NarratorGraphNode(15-15)HadithGraphNode(17-20)BaseGraphNode(3-7)
🪛 Ruff (0.12.2)
scripts/gen_search_index.py
43-43: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
🔇 Additional comments (7)
lib/types/graph-config.ts (1)
39-49: Extending GraphProps is a good move; watch for config type compatibility.
CustomGraphPropsnow extendsPartial<GraphProps<TNode, TLink>>while redefiningconfig: GraphViewConfig<TNode>. This is fine as long asGraphViewConfig<TNode>is assignable toGraphConfiguration<TNode, GraphLink>. Current structure appears compatible.Double-check TypeScript acceptability by ensuring no implicit any or incompatible fields are reported at Graph call sites (e.g., where
<Graph ... config={graphConfig} />is used).components/predecessors-successors-chain.tsx (3)
7-7: Type import is correct and keeps this file lean.
9-10: Router integration looks good.Importing and instantiating
useRouterenables the link-click navigation you added below.Also applies to: 525-526
571-586: Use Graph generics directly and drop the invalid alias.Replace the alias usage with the generic component. This also keeps props typing accurate (e.g.,
onClickLinksignature).Apply:
- <TypedGraph + <Graph<NarratorGraphNode, GraphLink> id="isnad" data={graphData} config={graphConfig} onClickLink={(source, target) => { const sourceNode = nodeMap.get(source); const targetNode = nodeMap.get(target); if (sourceNode && targetNode) { router.push( `/narrator/${encodeURIComponent(sourceNode.name)}/to/${encodeURIComponent(targetNode.name)}`, ); } }} />Likely an incorrect or invalid review comment.
app/narrator/[name]/to/[receiver]/page.tsx (1)
12-17: Verify whether pre-encoding params is desired; Next.js usually encodes for youReturning encoded values in generateStaticParams is non-standard; Next typically expects raw strings and will handle encoding. Since other routes in the codebase mix both approaches, ensure consistency to avoid double-encoding/decoding and unexpected URLs.
If you decide to return raw strings here, remove encodeURIComponent and keep the decoding in the page (or remove decoding everywhere for consistency).
pyproject.toml (1)
26-26: Nice addition of a Poe task for search index generationThe task wiring aligns with the new script and simplifies CI usage.
.github/workflows/deploy.yml (1)
81-83: Poe task & poethepoet present — no CI break expectedVerified pyproject.toml and the script entry: the Poe task and poethepoet dependency are declared and the script exists, so the workflow step should succeed (assuming CI installs project deps).
- pyproject.toml: [tool.poe.tasks] contains gen-search-index = { script = "scripts.gen_search_index:main", help = "Generate search index from database" } (line ~26)
- pyproject.toml: "poethepoet>=0.32.2" listed as a dependency (line ~13)
- scripts/gen_search_index.py exists and defines def main() (line ~28)
Optional (low priority): consider caching public/search_index.json keyed by data/sqlite.db, scripts/gen_search_index.py, and pyproject.toml to skip regeneration when inputs are unchanged.
AbdulrhmnGhanem
temporarily deployed
to
preview
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
components/network-graph.tsx (1)
15-17: TypedGraph alias resolved correctly.The type assertion approach properly addresses the TypeScript issue mentioned in past reviews, correctly typing the Graph component with the specific node and link types.
🧹 Nitpick comments (3)
components/predecessors-successors-chain.tsx (1)
59-77: Consider adding a comment to clarify the successor positioning logic.The positioning logic for successors on the left side of the graph is correct, but the intent might not be immediately clear to future maintainers.
- // Position successors on the left + // Position successors on the left (2 * horizontalSpacing away from center) successors.forEach((student, index) => {components/network-graph.tsx (1)
42-48: Consider making TypeScript errors more explicit.The
@ts-expect-errorcomments hide potential issues. Consider adding type guards or proper type definitions for the tailwind config.+// Type-safe color extraction from tailwind config +const getColorFromConfig = (path: string[]): string => { + let current: any = tailwindConfig; + for (const key of path) { + current = current?.[key]; + } + return current || '#000000'; // fallback color +}; + const COLORS = { - // @ts-expect-error color is defined in tailwind config - default: tailwindConfig.theme!.extend!.colors!.navy, - // @ts-expect-error color is defined in tailwind config - hover: tailwindConfig.theme!.extend!.colors!.parchment, - // @ts-expect-error color is defined in tailwind config - glow: tailwindConfig.theme!.extend!.colors!.gold, + default: getColorFromConfig(['theme', 'extend', 'colors', 'navy']), + hover: getColorFromConfig(['theme', 'extend', 'colors', 'parchment']), + glow: getColorFromConfig(['theme', 'extend', 'colors', 'gold']), };components/transmission-chain.tsx (1)
46-70: Complex positioning logic could benefit from extraction.The node positioning calculation is complex and could be extracted into a helper function for better readability and testability.
+function calculateNodeXPosition( + parentX: number | null, + position: number, + totalNodesAtLevel: number, + rootX: number, + horizontalOffset: number, + width: number +): number { + let xPosition; + if (totalNodesAtLevel === 1) { + xPosition = rootX; + } else { + xPosition = + parentX ?? + horizontalOffset + + (position * (width - 2 * horizontalOffset)) / + Math.max(totalNodesAtLevel - 1, 1); + } + return Math.max( + horizontalOffset, + Math.min(width - horizontalOffset, xPosition) + ); +} hadithData.transmissionChains.forEach((chain) => { let parentX: number | null = null; chain.narrators.forEach((narrator, index) => { if (!seenNodes.has(narrator.scholar_indx.toString())) { seenNodes.add(narrator.scholar_indx.toString()); const yPosition = topMargin + index * verticalSpacing; const nodesAtLevel = levelNodes.get(index) || []; const position = nodesAtLevel.indexOf(narrator.scholar_indx.toString()); const totalNodesAtLevel = nodesAtLevel.length; - let xPosition; - if (totalNodesAtLevel === 1) { - xPosition = rootX; - } else { - xPosition = - parentX ?? - horizontalOffset + - (position * (dimensions.width - 2 * horizontalOffset)) / - Math.max(totalNodesAtLevel - 1, 1); - } - - xPosition = Math.max( - horizontalOffset, - Math.min(dimensions.width - horizontalOffset, xPosition), - ); + const xPosition = calculateNodeXPosition( + parentX, + position, + totalNodesAtLevel, + rootX, + horizontalOffset, + dimensions.width + ); parentX = xPosition;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
app/narrator/[name]/to/[receiver]/page.tsx(1 hunks)components/network-graph.tsx(1 hunks)components/predecessors-successors-chain.tsx(3 hunks)components/transmission-chain.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/narrator/[name]/to/[receiver]/page.tsx
🧰 Additional context used
🧬 Code Graph Analysis (3)
components/network-graph.tsx (4)
lib/types/graph-config.ts (2)
CustomGraphProps(39-49)GraphViewConfig(4-37)lib/types/graph.ts (2)
NarratorGraphNode(15-15)GraphLink(9-13)components/network-workspace.tsx (2)
NetworkWorkspace(10-33)NetworkWorkspaceProps(6-8)components/narrator-card.tsx (1)
NarratorCard(109-142)
components/predecessors-successors-chain.tsx (1)
components/network-graph.tsx (1)
NetworkGraph(464-552)
components/transmission-chain.tsx (2)
lib/types/graph.ts (2)
NarratorGraphNode(15-15)GraphLink(9-13)components/network-graph.tsx (1)
NetworkGraph(464-552)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
🔇 Additional comments (5)
components/predecessors-successors-chain.tsx (2)
3-6: LGTM! Clean migration to NetworkGraph component.The imports have been properly updated to use the new NetworkGraph component, removing the previous Graph-based dependencies.
86-92: LGTM! Clean integration with NetworkGraph component.The component properly uses the NetworkGraph's callback pattern to provide dimension-aware graph data, with appropriate animation and directional settings.
components/transmission-chain.tsx (3)
3-5: LGTM! Clean import structure with proper type usage.The migration to NetworkGraph and NarratorGraphNode types is well organized.
33-33: Good responsive design practice.Using
dimensions.width / 2for centering the root node ensures proper layout across different screen sizes.
99-104: LGTM! Clean integration with NetworkGraph.The component properly uses the dimension-aware callback pattern and correctly configures the graph visualization settings.
AbdulrhmnGhanem
had a problem deploying
to
preview
GitHub Actions
Error
AbdulrhmnGhanem
force-pushed
the
ag/narattor-link
branch
from
a397353 to
2d07a99
Compare
AbdulrhmnGhanem
temporarily deployed
to
preview
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
lib/sqlite.ts (4)
53-57: Type shape looks good; consider future-proofing against name collisionsNarratorPair is minimal and fine. If your dataset contains homonymous narrators, aggregating by name will conflate distinct people. Consider including IDs (scholar_indx) alongside names, or switching grouping/filters to use IDs while still returning display names.
243-261: Tighten the WHERE clause for index-friendliness; optional numeric sort on hadith_noThe predicate
($source IS NULL OR h.source = $source)prevents efficient index use. Since the public API always supplies a Source, preferh.source = $source.Optionally, if hadith_no is a numeric string, cast when ordering to avoid lexicographic surprises (e.g., "10" < "2"). Only do this if consistent with the rest of the app.
Apply this diff to the statement:
statements.getHadithsFromNarratorToNarrator = db.prepare(` SELECT h.*, r1.name as narrator_name FROM hadiths h JOIN hadith_chains c1 ON h.source = c1.source AND h.chapter_no = c1.chapter_no AND h.hadith_no = c1.hadith_no JOIN hadith_chains c2 ON h.source = c2.source AND h.chapter_no = c2.chapter_no AND h.hadith_no = c2.hadith_no AND c2.position = c1.position + 1 JOIN rawis r1 ON c1.scholar_indx = r1.scholar_indx JOIN rawis r2 ON c2.scholar_indx = r2.scholar_indx WHERE r1.name = $from_narrator AND r2.name = $to_narrator - AND ($source IS NULL OR h.source = $source) - ORDER BY h.source, h.chapter_no, h.hadith_no + AND h.source = $source + ORDER BY h.source, h.chapter_no, h.hadith_no LIMIT $limit `);If you decide to enforce numeric ordering:
- ORDER BY h.source, h.chapter_no, h.hadith_no + ORDER BY h.source, h.chapter_no, CAST(h.hadith_no AS INTEGER)As a follow-up, consider these supporting indexes to speed the adjacent-pair joins:
-- outside this file, schema migration CREATE INDEX IF NOT EXISTS idx_chains_src_chap_hadith_pos ON hadith_chains(source, chapter_no, hadith_no, position); CREATE INDEX IF NOT EXISTS idx_chains_src_chap_hadith_scholar ON hadith_chains(source, chapter_no, hadith_no, scholar_indx);
262-276: Remove redundant DISTINCT/HAVING and use a sargable source predicateDISTINCT is redundant with GROUP BY, and
HAVING COUNT(*) > 0is always true post-GROUP BY. Also preferc1.source = $sourceover the nullable param pattern to keep the query sargable.Apply this diff:
statements.getNarratorPairs = db.prepare(` - SELECT DISTINCT r1.name as from_narrator, r2.name as to_narrator, COUNT(*) as hadith_count + SELECT r1.name as from_narrator, r2.name as to_narrator, COUNT(*) as hadith_count FROM hadith_chains c1 JOIN hadith_chains c2 ON c1.source = c2.source AND c1.chapter_no = c2.chapter_no AND c1.hadith_no = c2.hadith_no AND c2.position = c1.position + 1 JOIN rawis r1 ON c1.scholar_indx = r1.scholar_indx JOIN rawis r2 ON c2.scholar_indx = r2.scholar_indx - WHERE ($source IS NULL OR c1.source = $source) + WHERE c1.source = $source GROUP BY r1.name, r2.name - HAVING COUNT(*) > 0 ORDER BY hadith_count DESC `);If homonyms are a concern, group by scholar IDs and select names:
GROUP BY c1.scholar_indx, c2.scholar_indxThen add those IDs to the SELECT and to the NarratorPair type if/when needed.
440-453: Public API is coherent; consider light validationThe function signature and returned shape align with the SQL. Optionally, short-circuit on empty narrator names before hitting SQLite to avoid preparing work on invalid inputs.
tests/sqlite.test.ts (1)
173-187: Strengthen the assertions slightlyGood coverage. Add a simple length check to ensure at least one pair exists for the chosen source; it also guards against regressions that return an empty array.
Apply this diff:
test("getNarratorPairs returns narrator pairs with hadith counts", () => { const pairs = getNarratorPairs("Sahih Bukhari"); expect(pairs).toBeArray(); + expect(pairs.length).toBeGreaterThan(0);app/narrator/[name]/to/[receiver]/page.tsx (2)
19-37: Fix Next.js types: params is not a Promise; drop unnecessary awaitIn App Router, generateMetadata receives
{ params }synchronously. Typing params as Promise and awaiting it works at runtime but is misleading and weakens type safety.Apply this diff:
export async function generateMetadata({ params, }: { - params: Promise<{ name: string; receiver: string }>; + params: { name: string; receiver: string }; }): Promise<Metadata> { - const { name, receiver } = await params; + const { name, receiver } = params; const fromNarrator = decodeURIComponent(name); const toNarrator = decodeURIComponent(receiver);
39-94: Fix Next.js types in the page component; async unnecessarySame issue: params should not be a Promise. You also don't await anything after fixing that, so you can drop async.
Apply this diff:
-export default async function NarratorToNarratorPage({ +export default function NarratorToNarratorPage({ params, }: { - params: Promise<{ name: string; receiver: string }>; + params: { name: string; receiver: string }; }) { - const { name, receiver } = await params; + const { name, receiver } = params; const fromNarrator = decodeURIComponent(name); const toNarrator = decodeURIComponent(receiver); const hadiths = getHadithsFromNarratorToNarrator( fromNarrator, toNarrator, "Sahih Bukhari", );If generateStaticParams is updated to encode names, the decode here remains correct and consistent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
app/narrator/[name]/to/[receiver]/page.tsx(1 hunks)lib/sqlite.ts(4 hunks)tests/sqlite.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/sqlite.test.ts (1)
lib/sqlite.ts (3)
getNarrators(286-289)getHadithsFromNarratorToNarrator(440-453)getNarratorPairs(455-460)
app/narrator/[name]/to/[receiver]/page.tsx (2)
lib/sqlite.ts (2)
getNarratorPairs(455-460)getHadithsFromNarratorToNarrator(440-453)components/hadith-list.tsx (1)
HadithList(12-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
🔇 Additional comments (5)
lib/sqlite.ts (2)
87-89: Prepared statement registry updated correctlyNew statement slots are added consistently with the rest of the module and will be finalized in close(). No issues.
455-461: API symmetry with Source-anchored query looks goodFunction mirrors the statement contract correctly. No issues.
tests/sqlite.test.ts (1)
6-9: New imports are correct and scoped to new APIsImports match the new public functions in lib/sqlite.ts.
app/narrator/[name]/to/[receiver]/page.tsx (2)
1-9: Imports and routing utilities look goodNothing to flag here. Using notFound from next/navigation is appropriate for this route.
10-10: Static generation is fine; watch out for route explosionforce-static is OK, but see generateStaticParams note below—pre-rendering every pair could balloon build time/size if pairs are numerous.
Would you like to cap pre-rendering to the top N pairs (by hadith_count) or switch to on-demand ISR for the long tail?
AbdulrhmnGhanem
force-pushed
the
ag/narattor-link
branch
from
2d07a99 to
b27da9a
Compare
AbdulrhmnGhanem
had a problem deploying
to
preview
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
components/network-graph.tsx (2)
108-118: Good: path length error handling skips animation instead of using arbitrary fallbacks.This addresses the earlier concern about using magic defaults and avoids broken visuals.
188-216: Uniformly tag all cloned nodes for robust cleanup.You add
.animation-cloneonly toglowPath. TagbeamPathanddotPathtoo, so cleanup stays resilient to future ID changes.Apply this diff:
const beamPath = path.cloneNode() as SVGPathElement; beamPath.id = `beam-${pathId}`; + beamPath.classList.add("animation-clone"); @@ const dotPath = path.cloneNode() as SVGPathElement; dotPath.id = `dot-${pathId}`; + dotPath.classList.add("animation-clone");Your current cleanup already queries
.animation-clone, so this improves consistency.Also applies to: 218-231, 339-344
🧹 Nitpick comments (3)
components/network-graph.tsx (3)
40-48: Avoid importing Tailwind config in a client component; ship plain color constants or CSS variables instead.Importing
@/tailwind.configon the client pulls the entire config (and possibly plugins) into the bundle. That’s unnecessary bloat and can break if the config uses Node-only code. Prefer:
- Exporting a tiny color-constants module (tree‑shakable), or
- Using CSS variables defined in your Tailwind theme and reading them via styles.
Example (outside this diff): create
lib/ui/colors.tsand import from there, or define--color-navy,--color-parchment,--color-goldin:rootand reference them here viagetComputedStyle(document.documentElement).
479-481: Remove unused ref or leverage it for scoping.
graphRefis created but not used. Either remove it to satisfy linters, or use it to scope queries inuseAnimateLinksinstead ofdocument.getElementById.Apply this diff to remove it:
- const graphRef = useRef<HTMLDivElement>(null); @@ - <div id="isnad-graph-container" ref={graphRef}> + <div id="isnad-graph-container">Or thread the ref to
useAnimateLinksand query within that container.
66-74: Reduce console noise in production builds.Verbose
console.log/console.warncan flood logs and impact performance. Gate them behind a debug flag or strip in production.Example (outside this diff):
const DEBUG = process.env.NODE_ENV !== "production"; DEBUG && console.log("Animating path elements:", pathElements);Also applies to: 84-96, 119-129, 451-459
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
app/narrator/[name]/to/[receiver]/page.tsx(1 hunks)components/network-graph.tsx(1 hunks)lib/sqlite.ts(4 hunks)tests/sqlite.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/sqlite.test.ts
- app/narrator/[name]/to/[receiver]/page.tsx
- lib/sqlite.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-21T22:39:00.246Z
Learnt from: AbdulrhmnGhanem
PR: ElGarash/isnad#61
File: components/network-graph.tsx:476-489
Timestamp: 2025-08-21T22:39:00.246Z
Learning: In the isnad project, URL encoding for narrator names is intentionally inconsistent to work with GitHub Pages deployment. Some components use encodeURIComponent while others (like narrator-card.tsx) use raw names in URLs - this mixed approach is deliberate for GitHub Pages compatibility and should not be "fixed" to be consistent.
Applied to files:
components/network-graph.tsx
🧬 Code graph analysis (1)
components/network-graph.tsx (2)
components/network-workspace.tsx (2)
NetworkWorkspace(10-33)NetworkWorkspaceProps(6-8)lib/types/graph-config.ts (1)
GraphViewConfig(4-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
🔇 Additional comments (1)
components/network-graph.tsx (1)
473-498: LGTM: default link navigation encodes names appropriately.Using
encodeURIComponenthere matches the project’s deliberate mixed encoding approach for GitHub Pages compatibility noted previously. No change requested.
| // Add hover effect | ||
| path.addEventListener("mouseover", () => { | ||
| path.style.stroke = COLORS.hover; | ||
| path.style.strokeWidth = `${STROKE_WIDTH.hover}px`; | ||
| }); | ||
|
|
||
| path.addEventListener("mouseout", () => { | ||
| path.style.stroke = COLORS.default; | ||
| path.style.strokeWidth = `${STROKE_WIDTH.default}px`; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove per-path mouseover/mouseout listeners; rely on the global CSS hover you already inject.
You’re attaching event listeners every time animations are (re)initialized, but you never remove them. Over time, this stacks handlers and hurts performance. The global CSS inserted in addGlobalHoverStyles() already covers the hover effect.
Apply this diff to drop the redundant listeners:
- // Add hover effect
- path.addEventListener("mouseover", () => {
- path.style.stroke = COLORS.hover;
- path.style.strokeWidth = `${STROKE_WIDTH.hover}px`;
- });
-
- path.addEventListener("mouseout", () => {
- path.style.stroke = COLORS.default;
- path.style.strokeWidth = `${STROKE_WIDTH.default}px`;
- });If you must keep JS listeners, track them in a WeakMap and remove them in cleanupAnimations().
📝 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.
| // Add hover effect | |
| path.addEventListener("mouseover", () => { | |
| path.style.stroke = COLORS.hover; | |
| path.style.strokeWidth = `${STROKE_WIDTH.hover}px`; | |
| }); | |
| path.addEventListener("mouseout", () => { | |
| path.style.stroke = COLORS.default; | |
| path.style.strokeWidth = `${STROKE_WIDTH.default}px`; | |
| }); |
🤖 Prompt for AI Agents
In components/network-graph.tsx around lines 97 to 106, remove the per-path
"mouseover" and "mouseout" event listener attachments (the JS hover handlers)
because they are redundant with the global CSS hover injected by
addGlobalHoverStyles(); simply stop adding these listeners when (re)initializing
animations. If you absolutely need JS listeners, store each attached handler in
a WeakMap keyed by the path element and ensure you call removeEventListener for
those handlers from cleanupAnimations() before discarding or reinitializing
animations.
| // Create SVG gradient for the beam effect - unique ID for each path | ||
| const gradientId = `beam-gradient-${index}`; | ||
| let gradient = svg.querySelector(`#${gradientId}`) as SVGGradientElement; | ||
|
|
||
| // Only create gradient if it doesn't exist yet | ||
| if (!gradient) { | ||
| const svgNS = "http://www.w3.org/2000/svg"; | ||
| gradient = document.createElementNS(svgNS, "linearGradient"); | ||
| gradient.id = gradientId; | ||
| gradient.setAttribute("gradientUnits", "userSpaceOnUse"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make gradient IDs unique per path to avoid cross-SVG ID collisions.
Using beam-gradient-${index} risks duplicate IDs when multiple graphs exist or when animations restart. Key the gradient to the path ID.
Apply this diff:
- // Create SVG gradient for the beam effect - unique ID for each path
- const gradientId = `beam-gradient-${index}`;
+ // Create SVG gradient for the beam effect - unique ID per path
+ const pathId = path.id || `path-${index}`;
+ const gradientId = `beam-gradient-${pathId}`;
let gradient = svg.querySelector(`#${gradientId}`) as SVGGradientElement;And remove the duplicate pathId declaration below:
- const glowPath = path.cloneNode() as AnimatedPath;
- const pathId = path.id || `path-${index}`;
+ const glowPath = path.cloneNode() as AnimatedPath;Also applies to: 189-191
🤖 Prompt for AI Agents
In components/network-graph.tsx around lines 143-152 (and also apply the same
change at 189-191), the gradient ID currently uses `beam-gradient-${index}`
which can collide across multiple SVGs or restarts; change the gradient keying
to use the corresponding path's unique ID (e.g., `beam-gradient-${pathId}`) so
each gradient is globally unique per path, ensure you reference the existing
pathId variable (do not redeclare it) and remove the duplicate `pathId`
declaration found below, and update the other occurrence at lines 189-191 to the
same path-scoped gradientId pattern.
| // Add a cleanup function for when component unmounts | ||
| function cleanupAnimations() { | ||
| // Find all glow paths and cancel their animation frames | ||
| const glowPaths: NodeListOf<AnimatedElement> = document.querySelectorAll( | ||
| "[id^='glow-'], [id^='halo-'], [id^='trail-'], [id^='light-dot-']", | ||
| ); | ||
| glowPaths.forEach((path) => { | ||
| if (path._animationFrame) { | ||
| cancelAnimationFrame(path._animationFrame); | ||
| path._animationFrame = null; | ||
| } | ||
| }); | ||
|
|
||
| // Remove the added paths | ||
| const addedPaths = document.querySelectorAll( | ||
| ".animation-clone, [id^='glow-'], [id^='dot-'], [id^='halo-'], [id^='trail-'], [id^='light-dot-']", | ||
| ); | ||
| addedPaths.forEach((path) => path.parentNode?.removeChild(path)); | ||
|
|
||
| // Clear intervals from any original paths | ||
| const paths: NodeListOf<AnimatedPath> = document.querySelectorAll("path"); | ||
| paths.forEach((path) => { | ||
| if (path._animationInterval) { | ||
| clearInterval(path._animationInterval); | ||
| path._animationInterval = null; | ||
| } | ||
| }); | ||
|
|
||
| // Also remove any gradients we created | ||
| const gradients = document.querySelectorAll("[id^='beam-gradient-']"); | ||
| gradients.forEach((gradient) => { | ||
| if (gradient.parentNode) { | ||
| gradient.parentNode.removeChild(gradient); | ||
| } | ||
| }); | ||
| } // Add functionality to restart animations when paths come into view |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cleanup does not remove the per-path event listeners.
If you keep the JS hover listeners (see earlier comment), you must detach them here to prevent handler accumulation and unexpected hover behavior after multiple restarts.
If you decide to keep listeners, store them in a WeakMap<SVGPathElement, { over: (e: Event) => void; out: (e: Event) => void }> when attaching, and iterate that map here to call removeEventListener for each path.
🤖 Prompt for AI Agents
components/network-graph.tsx around lines 326 to 361: the cleanupAnimations
function currently cancels frames, clears intervals and removes DOM nodes but
does not remove per-path hover event listeners, causing handler accumulation on
restarts; update cleanupAnimations to iterate the WeakMap you should use to
store listeners (WeakMap<SVGPathElement, { over: (e: Event) => void; out: (e:
Event) => void }>) and for each entry call removeEventListener('mouseover',
over) and removeEventListener('mouseout', out) (also remove listeners from any
cloned/added paths if they were attached), then clear the WeakMap reference (or
reassign a new WeakMap) so listeners are fully detached and memory can be
reclaimed.
| function setupAnimationRestartOnScroll() { | ||
| // Store whether animations are currently running | ||
| const animationsActive = { value: true }; | ||
| let scrollTimeout: Timer | null = null; | ||
|
|
||
| // Function to check visibility and restart animations if needed | ||
| const checkVisibilityAndRestart = () => { | ||
| if (scrollTimeout) clearTimeout(scrollTimeout); | ||
| scrollTimeout = setTimeout(() => { | ||
| const isGraphVisible = () => { | ||
| const container = document.getElementById("isnad-graph-container"); | ||
| if (!container) return false; | ||
|
|
||
| const rect = container.getBoundingClientRect(); | ||
| const isVisible = rect.top < window.innerHeight && rect.bottom > 0; | ||
| return isVisible; | ||
| }; | ||
|
|
||
| if (isGraphVisible()) { | ||
| if (!animationsActive.value) { | ||
| // Restart animations by refreshing all paths | ||
| const paths = document.querySelectorAll(".link, path"); | ||
| if (paths.length) { | ||
| console.log("Graph is visible, restarting animations"); | ||
| cleanupAnimations(); | ||
| animatePathElements(paths as NodeListOf<Element>); | ||
| animationsActive.value = true; | ||
| } | ||
| } | ||
| } else { | ||
| // Graph is not visible, mark animations as inactive | ||
| animationsActive.value = false; | ||
| } | ||
| }, 100); | ||
| }; | ||
|
|
||
| // Set up scroll listener | ||
| window.addEventListener("scroll", checkVisibilityAndRestart, { | ||
| passive: true, | ||
| }); | ||
|
|
||
| // Clean up function | ||
| return () => { | ||
| window.removeEventListener("scroll", checkVisibilityAndRestart); | ||
| if (scrollTimeout) { | ||
| clearTimeout(scrollTimeout); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix timer type and pause animations when out of view.
- Type:
Timeris not a DOM type. UseReturnType<typeof setTimeout>for portability across Node/DOM. - Behavior: When the graph is out of view, you only flip a flag; animations keep running off-screen. Clean them up to save CPU/battery.
Apply this diff:
- let scrollTimeout: Timer | null = null;
+ let scrollTimeout: ReturnType<typeof setTimeout> | null = null;
@@
- } else {
- // Graph is not visible, mark animations as inactive
- animationsActive.value = false;
- }
+ } else {
+ // Graph is not visible; stop animations to save CPU/battery
+ if (animationsActive.value) {
+ cleanupAnimations();
+ animationsActive.value = false;
+ }
+ }📝 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.
| function setupAnimationRestartOnScroll() { | |
| // Store whether animations are currently running | |
| const animationsActive = { value: true }; | |
| let scrollTimeout: Timer | null = null; | |
| // Function to check visibility and restart animations if needed | |
| const checkVisibilityAndRestart = () => { | |
| if (scrollTimeout) clearTimeout(scrollTimeout); | |
| scrollTimeout = setTimeout(() => { | |
| const isGraphVisible = () => { | |
| const container = document.getElementById("isnad-graph-container"); | |
| if (!container) return false; | |
| const rect = container.getBoundingClientRect(); | |
| const isVisible = rect.top < window.innerHeight && rect.bottom > 0; | |
| return isVisible; | |
| }; | |
| if (isGraphVisible()) { | |
| if (!animationsActive.value) { | |
| // Restart animations by refreshing all paths | |
| const paths = document.querySelectorAll(".link, path"); | |
| if (paths.length) { | |
| console.log("Graph is visible, restarting animations"); | |
| cleanupAnimations(); | |
| animatePathElements(paths as NodeListOf<Element>); | |
| animationsActive.value = true; | |
| } | |
| } | |
| } else { | |
| // Graph is not visible, mark animations as inactive | |
| animationsActive.value = false; | |
| } | |
| }, 100); | |
| }; | |
| // Set up scroll listener | |
| window.addEventListener("scroll", checkVisibilityAndRestart, { | |
| passive: true, | |
| }); | |
| // Clean up function | |
| return () => { | |
| window.removeEventListener("scroll", checkVisibilityAndRestart); | |
| if (scrollTimeout) { | |
| clearTimeout(scrollTimeout); | |
| } | |
| }; | |
| } | |
| function setupAnimationRestartOnScroll() { | |
| // Store whether animations are currently running | |
| const animationsActive = { value: true }; | |
| let scrollTimeout: ReturnType<typeof setTimeout> | null = null; | |
| // Function to check visibility and restart animations if needed | |
| const checkVisibilityAndRestart = () => { | |
| if (scrollTimeout) clearTimeout(scrollTimeout); | |
| scrollTimeout = setTimeout(() => { | |
| const isGraphVisible = () => { | |
| const container = document.getElementById("isnad-graph-container"); | |
| if (!container) return false; | |
| const rect = container.getBoundingClientRect(); | |
| const isVisible = rect.top < window.innerHeight && rect.bottom > 0; | |
| return isVisible; | |
| }; | |
| if (isGraphVisible()) { | |
| if (!animationsActive.value) { | |
| // Restart animations by refreshing all paths | |
| const paths = document.querySelectorAll(".link, path"); | |
| if (paths.length) { | |
| console.log("Graph is visible, restarting animations"); | |
| cleanupAnimations(); | |
| animatePathElements(paths as NodeListOf<Element>); | |
| animationsActive.value = true; | |
| } | |
| } | |
| - } else { | |
| - // Graph is not visible, mark animations as inactive | |
| - animationsActive.value = false; | |
| } else { | |
| // Graph is not visible; stop animations to save CPU/battery | |
| if (animationsActive.value) { | |
| cleanupAnimations(); | |
| animationsActive.value = false; | |
| } | |
| } | |
| }, 100); | |
| }; | |
| // Set up scroll listener | |
| window.addEventListener("scroll", checkVisibilityAndRestart, { | |
| passive: true, | |
| }); | |
| // Clean up function | |
| return () => { | |
| window.removeEventListener("scroll", checkVisibilityAndRestart); | |
| if (scrollTimeout) { | |
| clearTimeout(scrollTimeout); | |
| } | |
| }; | |
| } |
🤖 Prompt for AI Agents
In components/network-graph.tsx around lines 362 to 410, the timer is typed as
non‑DOM `Timer` and the graph-only visibility handling leaves animations running
offscreen; change the timer type to use ReturnType<typeof setTimeout> for
cross‑environment correctness, and when the graph is not visible call
cleanupAnimations() and set animationsActive.value = false (and clear any
pending timeout) so animations are stopped while offscreen; keep the debounce
logic but ensure scrollTimeout is typed and cleared appropriately and the
cleanup returned by the function still removes the listener and clears the
timeout.
| const timer = setTimeout(() => { | ||
| console.log("Looking for links to animate"); | ||
|
|
||
| // Try multiple selector strategies to find the links | ||
| let links: NodeListOf<Element> | Element[] = | ||
| document.querySelectorAll(".link"); | ||
|
|
||
| if (links.length === 0) { | ||
| console.log("No .link elements found, trying direct path selector"); | ||
| const svgs = document.querySelectorAll("svg"); | ||
|
|
||
| if (svgs.length > 0) { | ||
| // Find all paths in all SVGs | ||
| const allPaths: Element[] = []; | ||
| svgs.forEach((svg) => { | ||
| const paths = svg.querySelectorAll("path"); | ||
| paths.forEach((path) => allPaths.push(path)); | ||
| }); | ||
|
|
||
| console.log(`Found ${allPaths.length} paths in SVGs`); | ||
|
|
||
| if (allPaths.length > 0) { | ||
| // Use all paths if we can't find specific link paths | ||
| links = allPaths; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| console.log(`Found ${links.length} elements to animate`); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Scope element queries to the graph container to avoid touching unrelated SVGs.
If .link elements aren’t found, the fallback currently queries every <svg> in document and may animate unrelated SVGs elsewhere on the page. Limit the search to #isnad-graph-container.
Apply this diff:
- // Try multiple selector strategies to find the links
- let links: NodeListOf<Element> | Element[] =
- document.querySelectorAll(".link");
+ // Try multiple selector strategies to find the links (scoped to container)
+ const container = document.getElementById("isnad-graph-container");
+ let links: NodeListOf<Element> | Element[] =
+ (container?.querySelectorAll(".link") ??
+ document.querySelectorAll(".link"));
@@
- const svgs = document.querySelectorAll("svg");
+ const svgs = container
+ ? container.querySelectorAll("svg")
+ : document.querySelectorAll("svg");Optionally, prefer a more specific selector like g.links path if available from the graph library.
📝 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.
| const timer = setTimeout(() => { | |
| console.log("Looking for links to animate"); | |
| // Try multiple selector strategies to find the links | |
| let links: NodeListOf<Element> | Element[] = | |
| document.querySelectorAll(".link"); | |
| if (links.length === 0) { | |
| console.log("No .link elements found, trying direct path selector"); | |
| const svgs = document.querySelectorAll("svg"); | |
| if (svgs.length > 0) { | |
| // Find all paths in all SVGs | |
| const allPaths: Element[] = []; | |
| svgs.forEach((svg) => { | |
| const paths = svg.querySelectorAll("path"); | |
| paths.forEach((path) => allPaths.push(path)); | |
| }); | |
| console.log(`Found ${allPaths.length} paths in SVGs`); | |
| if (allPaths.length > 0) { | |
| // Use all paths if we can't find specific link paths | |
| links = allPaths; | |
| } | |
| } | |
| } | |
| console.log(`Found ${links.length} elements to animate`); | |
| const timer = setTimeout(() => { | |
| console.log("Looking for links to animate"); | |
| // Try multiple selector strategies to find the links (scoped to container) | |
| const container = document.getElementById("isnad-graph-container"); | |
| let links: NodeListOf<Element> | Element[] = | |
| (container?.querySelectorAll(".link") ?? | |
| document.querySelectorAll(".link")); | |
| if (links.length === 0) { | |
| console.log("No .link elements found, trying direct path selector"); | |
| const svgs = container | |
| ? container.querySelectorAll("svg") | |
| : document.querySelectorAll("svg"); | |
| if (svgs.length > 0) { | |
| // Find all paths in all SVGs | |
| const allPaths: Element[] = []; | |
| svgs.forEach((svg) => { | |
| const paths = svg.querySelectorAll("path"); | |
| paths.forEach((path) => allPaths.push(path)); | |
| }); | |
| console.log(`Found ${allPaths.length} paths in SVGs`); | |
| if (allPaths.length > 0) { | |
| // Use all paths if we can't find specific link paths | |
| links = allPaths; | |
| } | |
| } | |
| } | |
| console.log(`Found ${links.length} elements to animate`); | |
| }, /* …delay… */); |
🤖 Prompt for AI Agents
In components/network-graph.tsx around lines 423 to 451, the fallback queries
every <svg> in document and may animate unrelated SVGs; change the DOM queries
to be scoped to the graph container element
(document.getElementById("isnad-graph-container") or similar) so you first run
container.querySelectorAll(".link") and, if none, search
container.querySelectorAll("svg") and collect paths only from those SVGs;
optionally prefer a more specific selector like
container.querySelectorAll("g.links path") to target link paths from the graph
library.
1 participant
Closes #31
Summary by CodeRabbit
New Features
Refactor
Chores
Tests