Skip to content

Update master with frontend changes in dev#492

Open
RevantCI wants to merge 19 commits into
masterfrom
dev
Open

Update master with frontend changes in dev#492
RevantCI wants to merge 19 commits into
masterfrom
dev

Conversation

@RevantCI

@RevantCI RevantCI commented May 26, 2026

Copy link
Copy Markdown
Collaborator

Update master with frontend changes in dev

Summary by CodeRabbit

  • New Features
    • Added verse-marker view/upload dialogs for ISL content, including single CSV and bulk ZIP flows with CSV/ZIP download.
    • ISL admin UI now supports an additional “vimeo url” metadata field.
  • Bug Fixes
    • Improved verse validation to use the latest stored versification configuration.
    • Bulk resource deletion now also cleans up related ISL video records.
    • Preview/Download UI now shows clearer loading states and disables actions while preparing.
  • Refactor / UI Updates
    • Updated ISL book name responses to use book_code (instead of language_id).
    • Resource pages now consistently scope data/mutations by content type.
  • Deployment
    • Frontend container now builds static assets and serves them via Nginx; Docker build context is optimized.

@RevantCI RevantCI requested review from AthulyaMS and KetanKBaboo May 26, 2026 11:30
@codeant-ai

codeant-ai Bot commented May 26, 2026

Copy link
Copy Markdown

Skipping CodeAnt AI review — this PR is a back-merge between long-lived branches (devmaster). The diff here has already been reviewed when the underlying commits landed on the source branch, so re-running analysis would produce duplicate findings on already-reviewed code.

If you want to analyze this anyway (e.g. you resolved conflicts with new logic), comment @codeant-ai : review and CodeAnt will start a review.

@vercel

vercel Bot commented May 26, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
vachan-admin-v2 Ready Ready Preview, Comment Jun 30, 2026 10:21am

Request Review

Tejaswini Rai and others added 2 commits May 28, 2026 11:06
Tejaswini Rai and others added 2 commits June 25, 2026 16:30
@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds database-backed ISL verse-marker storage and UI flows, updates book-code and Vimeo URL handling, scopes content resource queries by type, adjusts Bible selector loading behavior, and switches frontend delivery to an Nginx-based container setup.

Changes

ISL Verse Marker Workflow

Layer / File(s) Summary
Versification storage and validation
backend/app/db_models.py, backend/app/load_data.py, backend/app/crud/isl_verse_markers_crud.py, backend/app/crud/structural_crud.py
A new VersificationSchema model is added and seeded from JSON, marker validation reads the default schema from the database, and bulk resource deletion now includes IslVideo.
Book code and marker APIs
backend/app/schema.py, backend/app/crud/content_bible.py, frontend/src/utils/api.ts, frontend/src/hooks/useAPI.ts
BookNameResponse and the book-name response builder now expose book_code, and the frontend adds ISL verse-marker fetch, upload, update, and bulk-delete helpers plus React Query hooks.
Dialog props and view actions
frontend/src/utils/types.ts, frontend/src/components/ContentTypeAction.tsx, frontend/src/components/UploadOrViewDialog.tsx, frontend/src/components/VerseMarkerViewDialog.tsx
The generic upload/view dialog accepts extra view columns and actions, and the verse-marker view dialog renders the table, CSV download, delete flow, and upload entry point.
Verse marker upload dialog
frontend/src/components/VerseMarkerUploadDialog.tsx
Single CSV and bulk ZIP inputs are parsed into chapter entries, create/update requests are issued per entry, and the dialog renders per-book and per-chapter status summaries.
ISL page marker controls
frontend/src/pages/ISL.tsx, frontend/src/components/ContentTypeAction.tsx, frontend/src/components/VerseMarkerViewDialog.tsx, frontend/src/components/VerseMarkerUploadDialog.tsx, frontend/src/hooks/useAPI.ts
The ISL page adds cached verse-marker state, ZIP downloads, bulk upload actions, and passes the new view actions into the content dialog.
ISL table and payload mapping
frontend/src/pages/ISL.tsx
The ISL table switches to vimeo url, normalizes create/update payloads, adjusts metadata handling, adds a publish delay helper, and keeps admin-only table actions behind the role check.
Resource scope hooks
frontend/src/hooks/useAPI.ts, frontend/src/pages/Bibles.tsx, frontend/src/pages/Commentaries.tsx, frontend/src/pages/Dictionaries.tsx, frontend/src/pages/Infographics.tsx, frontend/src/pages/OBS.tsx, frontend/src/pages/Videos.tsx
Resource queries and mutations are scoped by content type, and the page row derivations adapt to the scoped data shapes.
Selector loading state
frontend/src/components/BibleSelector.tsx
useDownloadBibleContent is gated on dialog open and uploaded books, and the Preview/Download buttons reflect content-fetch loading state.

Frontend container runtime

Layer / File(s) Summary
Frontend image build
.dockerignore, docker/docker_frontend/Dockerfile, docker/docker_frontend/docker-compose.yml, docker/docker_frontend/nginx/frontend.conf
The frontend Docker build switches to a multi-stage Node-to-Nginx image, the compose service moves build inputs to build.args, the host port becomes 127.0.0.1:8081:80, and Nginx serves the static frontend output.
Dev-server host allowlist
backend/app/main.py, frontend/vite.config.ts
The backend CORS allowlist and Vite dev-server host allowlist add the new local and domain origins used by the frontend.

Sequence Diagram(s)

sequenceDiagram
  participant ISLPage as frontend/src/pages/ISL.tsx
  participant UploadDialog as VerseMarkerUploadDialog
  participant Hook as useUploadISLVerseMarkers
  participant Api as uploadISLVerseMarkers
  participant CRUD as backend/app/crud/isl_verse_markers_crud.py
  participant Schema as VersificationSchema
  ISLPage->>UploadDialog: open bulk upload
  UploadDialog->>Hook: submit parsed marker entries
  Hook->>Api: POST or PUT markers
  Api->>CRUD: /isl-verse-markers request
  CRUD->>Schema: read default versification
  CRUD-->>Hook: success or validation error
  Hook-->>UploadDialog: update per-video upload status
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

A bunny hopped through code all day,
With verses, zips, and bales of hay.
In Nginx burrows, neat and bright,
The markers loaded just right. 🐇

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 37.93% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title is generic and describes the branch flow more than the actual changes, so it does not clearly summarize the PR. Rename it to reflect the main change, such as the frontend ISL verse-marker and resource-scoping updates.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dev

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (1)
frontend/src/pages/ISL.tsx (1)

73-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reuse the shared book-name helpers instead of redefining them.

BOOK_CODE_TO_NAME and the toTitleCase/getBookName pair are duplicated here and again in VerseMarkerUploadDialog.tsx, while @/utils/books already exports BOOK_CODE_TO_NAME (and capitalizeBookName). Consolidating into the shared module avoids three diverging copies of the same mapping/casing logic.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/src/pages/ISL.tsx` around lines 73 - 85, The book-name mapping and
casing helpers are duplicated in ISL.tsx and VerseMarkerUploadDialog.tsx, which
risks the logic drifting apart. Remove the local BOOK_CODE_TO_NAME, toTitleCase,
and getBookName definitions from ISL.tsx and import the shared helpers from
`@/utils/books` instead, using BOOK_CODE_TO_NAME and capitalizeBookName wherever
book names are normalized or displayed. Update any call sites in ISL.tsx to use
the shared utility so both pages rely on the same source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/app/crud/isl_verse_markers_crud.py`:
- Around line 174-177: The marker validation path currently assumes the default
schema payload always contains maxVerses, so indexing versification["maxVerses"]
can raise a raw KeyError and surface as a 500. Update the CRUD flow around
max_verses_data to validate the schema shape first, check that versification
includes maxVerses before accessing it, and raise UnprocessableException with a
clear message when that field is missing or malformed.

In `@backend/app/load_data.py`:
- Around line 136-143: The seeding guard in the versification setup is too broad
and can skip creating the required default schema even when it is missing or
non-default. Update the logic around the existing English-ERV lookup in the
seeding path so it checks for the required default row using the same condition
as _get_versification() (is_default=True) instead of skipping based only on any
matching row or table presence, and ensure the default-versification seed is
created or retained whenever that specific default is absent.
- Around line 148-163: The versification seeding in load_versification_schema
currently catches every Exception and treats it as a duplicate insert, which
hides real DB failures. Update the try/except around db_.add, db_.commit, and
db_.rollback so only the expected unique-constraint/race condition is skipped,
and let other exceptions propagate or be logged as failures. Use the
VersificationSchema insert path as the locator and keep the “already seeded”
message only for the duplicate-insert case.

In `@backend/app/main.py`:
- Around line 55-56: The CORS origin allowlist in the main application only
covers localhost, so requests from the published frontend address on
127.0.0.1:8081 will be blocked. Update the CORS configuration in main.py where
the origin list is defined to include the actual published frontend origin
alongside http://localhost:8081 and the other existing entries. Make sure the
change is applied in the app startup/CORS setup so the frontend can reach the
API regardless of whether it is opened via localhost or the bound host address.

In `@docker/docker_frontend/Dockerfile`:
- Around line 29-41: The final Nginx stage in the Dockerfile still runs as root;
harden it by switching the runtime to a non-root user before the container
starts. Update the nginx stage around the existing COPY, EXPOSE, and CMD steps
to use the image’s unprivileged user (or create one), and make sure
/etc/nginx/conf.d/default.conf and /usr/share/nginx/html are readable by that
user. If needed, adjust the Nginx config to listen on an unprivileged port and
align EXPOSE/CMD with that runtime user.

In `@frontend/src/components/BibleSelector.tsx`:
- Around line 353-355: The Preview/Download loading state in BibleSelector
should not rely on isFetchingContent, since it can be true during background
refetches even when bibleContentData is already cached. Update the logic around
useDownloadBibleContent in BibleSelector to use an initial-load check such as
isPending or !bibleContentData for the hard loading state, and keep refetches
from disabling the actions or showing “Loading...” when content is already
available.

---

Nitpick comments:
In `@frontend/src/pages/ISL.tsx`:
- Around line 73-85: The book-name mapping and casing helpers are duplicated in
ISL.tsx and VerseMarkerUploadDialog.tsx, which risks the logic drifting apart.
Remove the local BOOK_CODE_TO_NAME, toTitleCase, and getBookName definitions
from ISL.tsx and import the shared helpers from `@/utils/books` instead, using
BOOK_CODE_TO_NAME and capitalizeBookName wherever book names are normalized or
displayed. Update any call sites in ISL.tsx to use the shared utility so both
pages rely on the same source of truth.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f6b40574-1846-4ff5-871f-adcfc739ee23

📥 Commits

Reviewing files that changed from the base of the PR and between 62f18c3 and a82c5bb.

📒 Files selected for processing (21)
  • .dockerignore
  • backend/app/crud/content_bible.py
  • backend/app/crud/isl_verse_markers_crud.py
  • backend/app/crud/structural_crud.py
  • backend/app/db_models.py
  • backend/app/load_data.py
  • backend/app/main.py
  • backend/app/schema.py
  • docker/docker_frontend/Dockerfile
  • docker/docker_frontend/docker-compose.yml
  • docker/docker_frontend/nginx/frontend.conf
  • frontend/src/components/BibleSelector.tsx
  • frontend/src/components/ContentTypeAction.tsx
  • frontend/src/components/UploadOrViewDialog.tsx
  • frontend/src/components/VerseMarkerUploadDialog.tsx
  • frontend/src/components/VerseMarkerViewDialog.tsx
  • frontend/src/hooks/useAPI.ts
  • frontend/src/pages/ISL.tsx
  • frontend/src/utils/api.ts
  • frontend/src/utils/types.ts
  • frontend/vite.config.ts

Comment on lines +174 to +177
max_verses_data = versification["maxVerses"].get(book_code)

if not max_verses_data:
raise UnprocessableException(
detail=f"No versification data for {book_code}"
)
raise UnprocessableException(detail=f"No versification data for {book_code}")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Validate the default schema shape before indexing maxVerses.

A malformed default row currently raises a raw KeyError here and turns marker validation into a 500. Return a controlled UnprocessableException when the DB schema payload is missing maxVerses.

Proposed fix
-    max_verses_data = versification["maxVerses"].get(book_code)
+    max_verses = (
+        versification.get("maxVerses")
+        if isinstance(versification, dict)
+        else None
+    )
+    if not isinstance(max_verses, dict):
+        raise UnprocessableException(
+            detail="Default versification schema is missing maxVerses"
+        )
+
+    max_verses_data = max_verses.get(book_code)
📝 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.

Suggested change
max_verses_data = versification["maxVerses"].get(book_code)
if not max_verses_data:
raise UnprocessableException(
detail=f"No versification data for {book_code}"
)
raise UnprocessableException(detail=f"No versification data for {book_code}")
max_verses = (
versification.get("maxVerses")
if isinstance(versification, dict)
else None
)
if not isinstance(max_verses, dict):
raise UnprocessableException(
detail="Default versification schema is missing maxVerses"
)
max_verses_data = max_verses.get(book_code)
if not max_verses_data:
raise UnprocessableException(detail=f"No versification data for {book_code}")
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/app/crud/isl_verse_markers_crud.py` around lines 174 - 177, The
marker validation path currently assumes the default schema payload always
contains maxVerses, so indexing versification["maxVerses"] can raise a raw
KeyError and surface as a 500. Update the CRUD flow around max_verses_data to
validate the schema shape first, check that versification includes maxVerses
before accessing it, and raise UnprocessableException with a clear message when
that field is missing or malformed.

Comment thread backend/app/load_data.py
Comment on lines +136 to +143
existing = db_.query(
db_models.VersificationSchema
).filter_by(
name="English-ERV"
).first()

if existing:
return

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Seed based on the required default row, not table emptiness.

_get_versification() only reads is_default=True, but this seeding path skips when any versification row exists, or when English-ERV exists without being default. That can leave marker create/update APIs without a usable schema.

Proposed fix
-    existing = db_.query(
+    existing_default = db_.query(
         db_models.VersificationSchema
     ).filter_by(
-        name="English-ERV"
+        is_default=True
     ).first()
 
-    if existing:
+    if existing_default:
         return
-        if session.query(db_models.VersificationSchema).count() == 0:
+        if not session.query(db_models.VersificationSchema).filter_by(is_default=True).first():
             json_file_versification = Path("data/versification.json").resolve()
-            populate_versification_table(session,str(json_file_versification))
+            populate_versification_table(session, str(json_file_versification))

Also applies to: 190-192

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/app/load_data.py` around lines 136 - 143, The seeding guard in the
versification setup is too broad and can skip creating the required default
schema even when it is missing or non-default. Update the logic around the
existing English-ERV lookup in the seeding path so it checks for the required
default row using the same condition as _get_versification() (is_default=True)
instead of skipping based only on any matching row or table presence, and ensure
the default-versification seed is created or retained whenever that specific
default is absent.

Comment thread backend/app/load_data.py
Comment on lines +148 to +163
try:
record = db_models.VersificationSchema(
name="English-ERV",
description=None,
data=data,
is_default=True,
)

db_.add(record)
db_.commit()

except Exception:
db_.rollback()
print(
"Versification schema already seeded by another process, skipping."
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Do not swallow all versification seed failures.

Only the duplicate-insert race should be skipped. Other DB errors are currently reported as “already seeded,” so the app can start without the required default schema and fail later in marker validation.

Proposed fix
+from sqlalchemy.exc import IntegrityError
+
     try:
         record = db_models.VersificationSchema(
             name="English-ERV",
             description=None,
             data=data,
@@
         db_.add(record)
         db_.commit()
 
-    except Exception:
+    except IntegrityError:
         db_.rollback()
         print(
             "Versification schema already seeded by another process, skipping."
         )
📝 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.

Suggested change
try:
record = db_models.VersificationSchema(
name="English-ERV",
description=None,
data=data,
is_default=True,
)
db_.add(record)
db_.commit()
except Exception:
db_.rollback()
print(
"Versification schema already seeded by another process, skipping."
)
from sqlalchemy.exc import IntegrityError
try:
record = db_models.VersificationSchema(
name="English-ERV",
description=None,
data=data,
is_default=True,
)
db_.add(record)
db_.commit()
except IntegrityError:
db_.rollback()
print(
"Versification schema already seeded by another process, skipping."
)
🧰 Tools
🪛 Ruff (0.15.18)

[warning] 159-159: Do not catch blind exception: Exception

(BLE001)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/app/load_data.py` around lines 148 - 163, The versification seeding
in load_versification_schema currently catches every Exception and treats it as
a duplicate insert, which hides real DB failures. Update the try/except around
db_.add, db_.commit, and db_.rollback so only the expected
unique-constraint/race condition is skipped, and let other exceptions propagate
or be logged as failures. Use the VersificationSchema insert path as the locator
and keep the “already seeded” message only for the duplicate-insert case.

Source: Linters/SAST tools

Comment thread backend/app/main.py
Comment on lines +55 to +56
"http://localhost:5173",
"http://localhost:8081",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Allow the actual published frontend origin as well.

docker/docker_frontend/docker-compose.yml Line 16 binds the app to 127.0.0.1:8081, but this list only allows http://localhost:8081. Opening the frontend via the published address will make the browser block API calls on CORS.

Suggested fix
 allowed_origins = [
     os.getenv("SUPERTOKENS_WEBSITE_DOMAIN", "http://localhost:5174"),
     # keep localhost for local dev
     "http://localhost:5174",
     "http://localhost:5173",
     "http://localhost:8081",
+    "http://127.0.0.1:8081",
 ]
📝 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.

Suggested change
"http://localhost:5173",
"http://localhost:8081",
"http://localhost:5173",
"http://localhost:8081",
"http://127.0.0.1:8081",
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/app/main.py` around lines 55 - 56, The CORS origin allowlist in the
main application only covers localhost, so requests from the published frontend
address on 127.0.0.1:8081 will be blocked. Update the CORS configuration in
main.py where the origin list is defined to include the actual published
frontend origin alongside http://localhost:8081 and the other existing entries.
Make sure the change is applied in the app startup/CORS setup so the frontend
can reach the API regardless of whether it is opened via localhost or the bound
host address.

Comment on lines +29 to +41
FROM nginx:alpine

# Remove default nginx config
RUN rm /etc/nginx/conf.d/default.conf

# Copy our nginx config
COPY docker/docker_frontend/nginx/frontend.conf /etc/nginx/conf.d/default.conf

# Copy built frontend
COPY --from=builder /app/dist /usr/share/nginx/html

CMD ["npm", "run", "dev", "--", "--host", "0.0.0.0"] No newline at end of file
EXPOSE 80
CMD ["nginx", "-g", "daemon off;"]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Run the Nginx stage as a non-root user.

The final image still runs as root. That unnecessarily increases the blast radius of any Nginx or container escape issue in a public-facing service.

Suggested hardening
 FROM nginx:alpine
 
 # Remove default nginx config
 RUN rm /etc/nginx/conf.d/default.conf
 
 # Copy our nginx config
 COPY docker/docker_frontend/nginx/frontend.conf /etc/nginx/conf.d/default.conf
 
 # Copy built frontend
 COPY --from=builder /app/dist /usr/share/nginx/html
+
+RUN chown -R nginx:nginx /usr/share/nginx/html /var/cache/nginx /var/log/nginx /etc/nginx/conf.d
+USER nginx
 
-EXPOSE 80
+EXPOSE 8080
 CMD ["nginx", "-g", "daemon off;"]
 server {
-    listen 80;
+    listen 8080;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docker/docker_frontend/Dockerfile` around lines 29 - 41, The final Nginx
stage in the Dockerfile still runs as root; harden it by switching the runtime
to a non-root user before the container starts. Update the nginx stage around
the existing COPY, EXPOSE, and CMD steps to use the image’s unprivileged user
(or create one), and make sure /etc/nginx/conf.d/default.conf and
/usr/share/nginx/html are readable by that user. If needed, adjust the Nginx
config to listen on an unprivileged port and align EXPOSE/CMD with that runtime
user.

Source: Linters/SAST tools

Comment on lines +353 to +355
const { data: bibleContentData, isFetching: isFetchingContent } = useDownloadBibleContent(
resourceId as number,
hasUploadedBooks,
isOpen && hasUploadedBooks,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

In TanStack Query v5, does useQuery().isFetchingbecome true during background refetches even when cacheddataalready exists, and isisPending/the absence of data the better signal for initial-load-only UI disabling?

💡 Result:

In TanStack Query v5, isFetching becomes true whenever a query function is executing, which includes both initial loads (when no data exists) and background refetches (when cached data already exists) [1][2][3]. Regarding your question about initial-load-only UI states, checking isPending is indeed the better and recommended approach [4]. Here is the breakdown of why this is the preferred signal: 1. isPending: This state is derived from the query status ('pending') and indicates that the query has no data yet [1][5][4]. It is the standard signal for displaying "hard" loading states, such as full-page spinners or skeleton screens, because it guarantees that no data is available to display [4][6]. 2. isFetching: Because this flag is true during both initial loads and background refetches, using it alone for an initial-load UI would cause your "hard" loading indicator to flicker or reappear every time a background refetch occurs (e.g., polling or window focus refetching) [7][6][8]. If you need to distinguish between these states, you can use the following patterns: - For initial-load-only UI: Use isPending [4][6]. - For background-only refetch indicators: Use isRefetching [1][3]. This flag is explicitly true only when a background refetch is in-flight and does not include the initial pending state (it is equivalent to isFetching &&!isPending) [1][5]. For TypeScript users, checking isPending is also advantageous because it allows the compiler to correctly narrow the data type, confirming that data is defined once the check passes [4].

Citations:


Use an initial-load flag instead of isFetchingContent.

isFetchingContent also becomes true during background refetches, so Preview/Download can stay disabled or show “Loading…” even when cached bibleContentData is already available. Use isPending or !bibleContentData for the hard-loading state here.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/src/components/BibleSelector.tsx` around lines 353 - 355, The
Preview/Download loading state in BibleSelector should not rely on
isFetchingContent, since it can be true during background refetches even when
bibleContentData is already cached. Update the logic around
useDownloadBibleContent in BibleSelector to use an initial-load check such as
isPending or !bibleContentData for the hard loading state, and keep refetches
from disabling the actions or showing “Loading...” when content is already
available.

KetanKBaboo and others added 2 commits June 29, 2026 10:48

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@frontend/src/utils/api.ts`:
- Around line 137-151: `fetchResources` is hardcoded to request only the first
500 items, which can truncate larger resource lists without warning. Update this
API helper to avoid relying on a fixed `page_size` cap by either fetching
additional pages until all resources are collected or using a server-provided
total/count to drive pagination, while keeping the `contentType` filtering and
`404` handling in `fetchResources` intact.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7c40cb04-5ff5-4f3a-a0f4-07a0df2954d6

📥 Commits

Reviewing files that changed from the base of the PR and between a82c5bb and e605292.

📒 Files selected for processing (9)
  • frontend/src/hooks/useAPI.ts
  • frontend/src/pages/Bibles.tsx
  • frontend/src/pages/Commentaries.tsx
  • frontend/src/pages/Dictionaries.tsx
  • frontend/src/pages/ISL.tsx
  • frontend/src/pages/Infographics.tsx
  • frontend/src/pages/OBS.tsx
  • frontend/src/pages/Videos.tsx
  • frontend/src/utils/api.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/src/pages/ISL.tsx

Comment thread frontend/src/utils/api.ts
Comment on lines +137 to 151
export const fetchResources = async (contentType?: string) => {
try {
const res = await API.get("/resources");
const res = await API.get("/resources", {
params: {
...(contentType ? { content_type: contentType } : {}),
page: 0,
page_size: 500,
},
});
return Array.isArray(res.data) ? res.data : [];
} catch (err: any) {
if (err.response?.status === 404) {
return [];
}
if (err.response?.status === 404) return [];
throw err;
}
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win

Hardcoded page_size: 500 can silently truncate results.

fetchResources always requests page: 0, page_size: 500 with no follow-up paging. Any content type with more than 500 resources will be silently capped, and the affected page (Bibles/Videos/etc.) will render an incomplete list with no error. Consider paginating or raising the cap based on a count, rather than relying on a fixed limit.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/src/utils/api.ts` around lines 137 - 151, `fetchResources` is
hardcoded to request only the first 500 items, which can truncate larger
resource lists without warning. Update this API helper to avoid relying on a
fixed `page_size` cap by either fetching additional pages until all resources
are collected or using a server-provided total/count to drive pagination, while
keeping the `contentType` filtering and `404` handling in `fetchResources`
intact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants