[Feature] implement optimization and load on scroll for posts#31
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds paginated posts API and scroll-based lazy-loading, refactors per-video initialization into Changes
Sequence DiagramsequenceDiagram
actor User
participant Browser
participant Flask
participant DB
participant HLS as HLS.js
User->>Browser: Open homepage
Browser->>Flask: GET /
Flask->>DB: Fetch first POSTS_PER_PAGE posts
DB-->>Flask: Posts + metadata
Flask-->>Browser: Render page with initial posts
Note over Browser: User scrolls near bottom
Browser->>Browser: scroll listener triggers
Browser->>Flask: GET /api/posts?page=2
Flask->>DB: Query posts with LIMIT/OFFSET
DB-->>Flask: Page posts
Flask-->>Browser: JSON {posts, has_next}
alt has_next = true
Browser->>Browser: Render posts into `#posts-container`
Browser->>Flask: GET /api/check_admin
Flask-->>Browser: {is_admin}
Browser->>Browser: Add Edit/Delete if admin
alt post has video
Browser->>Browser: create video container
Browser->>Browser: call initializeVideoPlayer(container)
Browser->>HLS: (if needed) load manifest / levels
HLS-->>Browser: quality levels
Browser->>Browser: build quality menu & wire controls
end
else has_next = false
Browser->>Browser: show `#end-of-posts-message`
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
static/js/video_player.js (2)
16-33: Harden control element lookups to avoid null dereferences
initializeVideoPlayerassumes all queried controls (playPauseBtn,muteBtn,volumeSlider,fullscreenBtn, etc.) exist; any divergence between HTML markup (includingcomponents/video_player.htmland the dynamic markup instatic/js/script.js) and these selectors will cause immediate runtime errors (e.g.,playPauseBtn.querySelectoronnull, ormuteBtn.addEventListenerwhenmuteBtnisnull).Consider validating critical elements up front and bailing out (with a clear console warning) if any are missing, or at least guarding subsequent usage:
function initializeVideoPlayer(container) { - const playOverlay = container.querySelector(".play-overlay"); - const playPauseBtn = container.querySelector(".play-pause-btn"); - const playPauseIcon = playPauseBtn.querySelector(".play-icon"); + const playOverlay = container.querySelector(".play-overlay"); + const playPauseBtn = container.querySelector(".play-pause-btn"); + const playPauseIcon = playPauseBtn && playPauseBtn.querySelector(".play-icon"); // ...similar for other controls... + if (!playOverlay || !playPauseBtn || !playPauseIcon || !progressBar || !muteBtn || !volumeSlider || !fullscreenBtn) { + console.warn("Video controls markup incomplete for container:", container); + return; + }This prevents a single malformed container from breaking video initialization globally.
Also applies to: 276-301
302-307: Multiple document‑level listeners per player could be centralized laterEach player instance registers its own
fullscreenchange(and vendor variants) anddocument.clickhandlers. Functionally this works, but on pages with many videos it can add unnecessary overhead and duplication.In a follow‑up, consider centralizing these document‑level listeners once (e.g., maintaining a registry of players or active menus) rather than per container. Not urgent, but worth keeping in mind if the number of players grows.
Also applies to: 338-345
templates/index.html (1)
51-76: Consider removing nested<a>elements in post markupThe post markup wraps everything in an
<a class="post-button">and also contains inner<a>tags (for the video player wrapper and the Edit/Delete links). This is invalid HTML and can lead to inconsistent click behavior and accessibility issues. The lazy-loaded posts instatic/js/script.jsmirror this pattern.When convenient, consider restructuring to have a single outer clickable block (e.g., wrapping the content in a block element with a click handler or using only one anchor per post) and moving Edit/Delete controls outside that main link.
static/js/script.js (3)
150-158: Re‑initialize image viewer for lazily loaded galleriesNew posts appended via
loadMorePostscan include:imageHTML = ` <div class="gallery"> <img src="${post.image}" alt="Post Image" class="image" loading="lazy"> </div> `;But the existing Viewer.js setup only runs once on
DOMContentLoadedover the initial.galleryelements, so newly loaded galleries will not get the enhanced viewer behavior.Consider extracting the viewer initialization into a reusable function and calling it for newly added galleries, e.g.:
-document.addEventListener('DOMContentLoaded', function () { - const galleries = document.querySelectorAll('.gallery'); - galleries.forEach(gallery => { /* init Viewer */ }); -}); +function initGalleries(root = document) { + const galleries = root.querySelectorAll('.gallery'); + galleries.forEach(gallery => { /* init Viewer */ }); +} + +document.addEventListener('DOMContentLoaded', () => initGalleries(document));And inside
loadMorePostsafterpostsContainer.appendChild(postLink):+ initGalleries(postLink);This keeps UX consistent between initial and lazy‑loaded content.
Also applies to: 224-237
107-125: Add basic null‑safety for loading/end indicatorsWithin the lazy‑load handler you assume
loadingIndicatorandendOfPostsMessageexist wheneverpostsContainerexists:const loadingIndicator = document.getElementById('loading-indicator'); const endOfPostsMessage = document.getElementById('end-of-posts-message'); // ... loadingIndicator.style.display = 'flex';If a future template forgets to include one of these elements but still uses
id="posts-container", this will throw aTypeError.A small defensive guard would make this more robust:
- if (!postsContainer) return; + if (!postsContainer) return; + + if (!loadingIndicator || !endOfPostsMessage) { + console.warn("Lazy loading: missing loading or end-of-posts elements"); + }And when toggling styles, check for existence before accessing
.style. Not urgent, but it hardens the code against template drift.Also applies to: 254-260
133-148: Consider caching admin status instead of calling/api/check_adminon every pageFor each page of posts you do:
const isAdmin = await fetch('/api/check_admin') .then(res => res.json()) .then(resData => resData.is_admin) .catch(() => false);Admin status is session‑scoped and effectively static for the duration of the page, so this extra network roundtrip on every lazy‑load is avoidable.
You could cache it once at the top‑level (or on first call) and reuse:
let cachedIsAdmin = null; async function getIsAdmin() { if (cachedIsAdmin !== null) return cachedIsAdmin; try { const res = await fetch('/api/check_admin'); const data = await res.json(); cachedIsAdmin = !!data.is_admin; } catch { cachedIsAdmin = false; } return cachedIsAdmin; } // in loadMorePosts const isAdmin = await getIsAdmin();This reduces latency and server load, especially when scrolling through many pages.
app.py (1)
773-808: Reusefetch_video_datainedit_postGET branch to avoid duplicationThe
edit_postGET logic reimplements video lookup logic that now lives infetch_video_data:if post["video_id"]: video_id = post["video_id"] # repeated Supabase query building videodata dict...You can simplify and keep behavior consistent by delegating to
fetch_video_data:- if post["video_id"]: - video_id = post["video_id"] - try: - video_resp = supabase_client.table("videos")... - # build videodata - post["video"] = videodata - except Exception as e: - print(f"Error fetching video info for video_id={video_id}: {e}") + video_id = post.get("video_id") + if video_id: + post["video"] = fetch_video_data(video_id)This removes duplicate Supabase access patterns and centralizes any future changes to video metadata.
Also applies to: 180-201
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app.py(8 hunks)static/css/style.css(1 hunks)static/js/script.js(1 hunks)static/js/video_player.js(1 hunks)templates/index.html(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app.py (1)
static/js/script.js (3)
data(21-21)data(32-32)page(113-113)
⏰ 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: test
🔇 Additional comments (5)
static/css/style.css (1)
725-742: Loader styles look good and self‑containedThe new
.loaderand@keyframes l3are syntactically correct, scoped, and reuse existing theme variables without impacting prior styles. Degradation in browsers lackingmask-compositewill be graceful.templates/index.html (1)
5-5: Markup wiring for lazy loading looks correctThe
id="posts-container",#loading-indicator, and#end-of-posts-messageadditions line up with the selectors used instatic/js/script.jsand the.loaderstyles instatic/css/style.css. Structurally this should work well with the new infinite scroll behavior.Also applies to: 79-85
app.py (3)
180-201: Good reuse offetch_video_datahelperCentralizing video metadata access in
fetch_video_dataand reusing it inhome/view_postreduces duplication and keeps the shape of thevideoobject consistent across views and the API.
246-254: Consistent video data wiring into templatesUsing
fetch_video_data(video_id)in bothhome(tuple element) andview_post(post["video"]) ensures templates receive a uniformvideostructure whether rendered server‑side or via the JSON API. This is a clean improvement over scattered inline queries.Also applies to: 839-878
975-981:/api/check_adminimplementation is straightforward and matches frontend expectationsThe new
/api/check_adminendpoint cleanly exposes{"is_admin": bool}based on the session, which lines up with howstatic/js/script.jsconsumes it to decide whether to render Edit/Delete links for lazy-loaded posts.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
static/js/script.js (1)
84-103: Re-initialize image viewer for lazily loaded.galleryelementsThe viewer setup at the top of this file runs only once on
DOMContentLoaded, so.galleryelements injected by lazy loading will not get the enhanced viewer behavior.After appending each
postLink, you can initialize viewers for any new galleries in that post, preferably by extracting the existing viewer initialization into a reusable helper:- postsContainer.appendChild(postLink); + postsContainer.appendChild(postLink); + + // Initialize viewer.js for any new galleries in this post + const newGalleries = postLink.querySelectorAll('.gallery'); + newGalleries.forEach(gallery => { + const image = gallery.querySelector('img'); + if (!image) return; + const viewer = new Viewer(gallery, { + movable: true, + zoomable: true, + fullscreen: true, + toolbar: false, + }); + image.addEventListener('click', (event) => { + event.preventDefault(); + event.stopPropagation(); + viewer.show(); + }); + });You can also DRY this up by moving the viewer initialization logic into a shared function used both on initial load and here.
Also applies to: 150-158, 237-245
🧹 Nitpick comments (4)
static/js/script.js (1)
133-137: Cache admin status instead of calling/api/check_adminon every page fetch
/api/check_adminis called on everyloadMorePostsinvocation, even though admin status is effectively per-session. You can avoid redundant network calls by caching the value after the first check:- let page = 2; // Start with page 2, since page 1 is loaded initially + let page = 2; // Start with page 2, since page 1 is loaded initially let isLoading = false; let hasNext = true; + let isAdminCache = null; ... - const isAdmin = await fetch('/api/check_admin') - .then(res => res.json()) - .then(resData => resData.is_admin) - .catch(() => false); + if (isAdminCache === null) { + isAdminCache = await fetch('/api/check_admin') + .then(res => res.json()) + .then(resData => resData.is_admin) + .catch(() => false); + } + const isAdmin = isAdminCache;This keeps behavior the same while cutting unnecessary requests as the user scrolls.
Also applies to: 246-247
app.py (2)
953-975: Make/api/postsordering resilient ifTIMESTAMP_FIELDis ever unset
/api/postscurrently does:res = supabase_client.table("posts").select(select_fields)\ .order(TIMESTAMP_FIELD, desc=True)\ .offset(offset).limit(POSTS_PER_PAGE).execute()Elsewhere (
home(),filter_posts()), you explicitly fall back to ordering by"id"whenTIMESTAMP_FIELDis not resolved. Even thoughresolve_timestamp_field()currently guarantees a non-Nonevalue, mirroring that pattern here will make the endpoint more robust to future changes and to schema/probe failures.For example:
- res = supabase_client.table("posts").select(select_fields).order(TIMESTAMP_FIELD, desc=True).offset(offset).limit(POSTS_PER_PAGE).execute() + order_field = TIMESTAMP_FIELD if TIMESTAMP_FIELD else "id" + res = ( + supabase_client.table("posts") + .select(select_fields) + .order(order_field, desc=True) + .offset(offset) + .limit(POSTS_PER_PAGE) + .execute() + )You may also want to reuse a small helper for the timestamp formatting logic to keep behavior identical with
home()/filter_posts().
953-975: Be aware of N+1 queries when enriching posts with video dataInside
/api/posts, each post with avideo_idtriggers a separatefetch_video_data(video_id)call. For pages with many video posts, this becomes an N+1 pattern against thevideostable.This is acceptable for low volume, but if you expect many posts per page and frequent scrolling, consider a future optimization that batches video lookups (e.g., collecting all
video_ids for the page and fetching them in one query, then mapping by id) while still reusing thefetch_video_datashape.Also applies to: 968-972
static/js/video_player.js (1)
302-307: Consider de-duplicating global event listeners across video instancesEach call to
initializeVideoPlayeradds its owndocument-level listeners for fullscreen change and global click (to close menus). With many video containers on a long page, these listeners multiply and all run on every relevant event.Behavior is correct, but you could simplify and reduce overhead by registering these document-level handlers once (outside
initializeVideoPlayer) and having them delegate to the active/fullscreen container as needed. Not urgent, but worth considering if the number of videos grows.Also applies to: 338-345
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app.py(8 hunks)static/js/script.js(1 hunks)static/js/video_player.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app.py (1)
static/js/script.js (3)
data(21-21)data(32-32)page(113-113)
⏰ 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: test
🔇 Additional comments (4)
static/js/script.js (1)
105-125: Lazy-loading flow and loading state management look solidThe paging state (
page,isLoading,hasNext), scroll trigger, andtry/finallyaround the fetch give you a clean, race-free lazy-loading behavior and ensure the loading indicator is always cleared. This aligns well with the backend/api/postscontract and the PR’s performance goals.Also applies to: 254-260
app.py (2)
98-99: Good consolidation of video data fetching and pagination constantUsing
POSTS_PER_PAGEin bothhome()and/api/posts, combined with thefetch_video_datahelper reused inhome(),view_post(), and the API, keeps pagination and video metadata consistent across views and the JSON API. This is a nice reduction of duplication and makes it easier to adjust page size or video fields in one place.Also applies to: 180-199, 232-255, 869-878
885-889: Admin JSON endpoints look consistent and minimally exposedThe updated
/admin/inspectresponse (JSON withis_adminand a masked key) and the new/api/check_adminroute cleanly separate admin state probing from the main views. Returning{"is_admin": False, "error": "admin only"}with 403 for unauthenticated calls is sensible, and/api/check_adminexposes only a boolean flag, which is appropriate for the client checks instatic/js/script.js.Also applies to: 923-926, 986-992
static/js/video_player.js (1)
1-4: Per-containerinitializeVideoPlayerabstraction is well-structured and works for dynamically added videosThe new
initializeVideoPlayer(container)plus the DOMContentLoaded bootstrap (and calls fromscript.jsfor lazily appended posts) give you a clean, reusable initialization path. The helper functions for controls, HLS/native playback selection, and control visibility keep the logic manageable, and the guard around theHlsglobal avoids ReferenceErrors when the library is unavailable.Also applies to: 6-37, 217-275, 276-354
| const loadingIndicator = document.getElementById('loading-indicator'); | ||
| const endOfPostsMessage = document.getElementById('end-of-posts-message'); | ||
|
|
There was a problem hiding this comment.
Guard against missing loading/end-of-posts elements to avoid runtime errors
loadingIndicator and endOfPostsMessage are assumed to exist; if the DOM ever omits one (e.g., template reuse), loadingIndicator.style... or endOfPostsMessage.style... will throw and break lazy loading.
You can harden this by bailing out when any of the required nodes is missing:
document.addEventListener('DOMContentLoaded', () => {
- const postsContainer = document.getElementById('posts-container');
- const loadingIndicator = document.getElementById('loading-indicator');
- const endOfPostsMessage = document.getElementById('end-of-posts-message');
-
- if (!postsContainer) return;
+ const postsContainer = document.getElementById('posts-container');
+ const loadingIndicator = document.getElementById('loading-indicator');
+ const endOfPostsMessage = document.getElementById('end-of-posts-message');
+
+ if (!postsContainer || !loadingIndicator || !endOfPostsMessage) {
+ return;
+ }Also applies to: 120-125, 249-252, 258-260
🤖 Prompt for AI Agents
In static/js/script.js around lines 108-110 (and similarly at 120-125, 249-252,
258-260), the code assumes document.getElementById returned non-null for
loadingIndicator and endOfPostsMessage and will throw when accessing .style on a
missing element; fix by null-checking each DOM node after retrieval and bail out
or skip the related logic when either is missing (e.g., return early from the
function or guard conditional before manipulating .style or adding/removing
classes), ensuring all places that reference these elements perform the same
presence checks.
| if (data.posts && data.posts.length > 0) { | ||
| const isAdmin = await fetch('/api/check_admin') | ||
| .then(res => res.json()) | ||
| .then(resData => resData.is_admin) | ||
| .catch(() => false); |
There was a problem hiding this comment.
Fix nested <a> tags and admin links inside the main post link
The generated HTML nests <a> tags inside another <a> (adminLinks and the extra <a> around ${videoHTML}), which is invalid HTML and can cause confusing click behavior (e.g., edit/delete links triggering the main post navigation).
Consider keeping the main card as a single link and rendering admin controls outside of it:
- let adminLinks = '';
- if (isAdmin) {
- adminLinks = `
- <a style="padding-right: 5px;" href="/edit/${post.id}">Edit</a>
- <a href="/delete/${post.id}">Delete</a>
- `;
- }
+ let adminLinks = '';
+ if (isAdmin) {
+ adminLinks = `
+ <a style="padding-right: 5px;" href="/edit/${post.id}">Edit</a>
+ <a href="/delete/${post.id}">Delete</a>
+ `;
+ }
...
- postLink.innerHTML = `
- <a href="/post/${post.id}" class="post-button">
- <h1 class="Heding">${post.title}</h1>
- <p class="text">${post.content}</p>
- ${imageHTML}
- <a>
- ${videoHTML}
- </a>
- <small>Posted on ${post.formatted_timestamp || (post.timestamp ? new Date(post.timestamp).toLocaleDateString() : '')}</small>
- <br>
- ${adminLinks}
- </a>
- `;
+ postLink.innerHTML = `
+ <a href="/post/${post.id}" class="post-button">
+ <h1 class="Heding">${post.title}</h1>
+ <p class="text">${post.content}</p>
+ ${imageHTML}
+ ${videoHTML}
+ <small>Posted on ${post.formatted_timestamp || (post.timestamp ? new Date(post.timestamp).toLocaleDateString() : '')}</small>
+ </a>
+ <br>
+ ${adminLinks}
+ `;This keeps the entire card clickable while ensuring admin links are independent anchors.
Also applies to: 142-148, 224-236
🤖 Prompt for AI Agents
In static/js/script.js around lines 133-137 (and also address similar patterns
at 142-148 and 224-236), the generated markup currently nests anchor tags
(adminLinks and an extra anchor wrapping videoHTML) inside the main post anchor
which is invalid and causes click conflicts; fix this by keeping the main card
as a single top-level anchor or clickable container and render admin controls
(edit/delete links) outside that anchor: remove any inner <a> wrappers around
videoHTML, move adminLinks out of the main link container (place them as sibling
elements), and if you need the card to remain fully clickable attach a click
handler or wrap non-admin content in the main anchor while ensuring admin
anchors are not inside it so their clicks don't bubble to post navigation.
Ensure identical fixes are applied to the other mentioned line ranges.
AmanatAliPanhwer
deleted the
feat/26-implement-optimization-and-load-on-scroll-for-posts
branch
AmanatAliPanhwer
restored the
feat/26-implement-optimization-and-load-on-scroll-for-posts
branch
1 participant
📌 Summary
Optimize application performance by implementing server-side pagination and a client-side load-on-scroll mechanism for the blog post list. This reduces the initial data load by only fetching the first page of posts and dynamically loading more as the user scrolls.
🔧 Type of Change
🧩 Related Issues
Closes #26
🧠 Changes Introduced
app.py):indexroute to fetch only the first page of posts (limit 10)./api/postsendpoint to serve subsequent pages via JSON, accepting apagequery parameter.id="posts-container"to the main posts wrapper for easier DOM manipulation.#loading-indicator) and end-of-posts message (#end-of-posts-message).loadMorePostsfunction to fetch data from the new API and append posts dynamically to the DOM.📸 Screenshots (if applicable)
N/A
✅ Checklist
🧾 Additional Notes
This implementation significantly improves the initial load time for the homepage by deferring the loading of older posts.
Summary by CodeRabbit
New Features
UI/UX Improvements
Admin
✏️ Tip: You can customize this high-level summary in your review settings.