feat: configuration type and feature to disable tomorrow time #44
Conversation
… the google sheet config data, add option to enable/disable tomorrow salah times - added prettier as dev dependency to ensure formatting stays consistent
|
@mahfuzhannan is attempting to deploy a commit to the MosqueOS Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds a runtime ConfigurationJson (type, defaults, provider, hook), loads merged config at app root, gates Announcement and tomorrow-prayer UI via feature flags, extends Google Sheets + REST data retrieval with service-account toggle and caching, introduces an embeddable TodayPrayerTime widget and admin embed UI, and replaces moment/moment-hijri with Luxon-based datetime utilities. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser
participant App as app/page.tsx
participant DataSvc as MosqueDataService
participant Sheets as GoogleSheetsService
participant Lib as ConfigUtils
participant Provider as ConfigurationProvider
participant UI as Component (Announcement / PrayerTimes)
Browser->>App: Request page
App->>DataSvc: getConfiguration()
alt Sheets enabled (env present)
DataSvc->>Sheets: sheetsGetConfigurationData()
Sheets-->>DataSvc: rows
DataSvc->>Lib: deepmerge(configurationDefaults, fetchedConfig)
else Sheets disabled / API fallback
DataSvc->>DataSvc: fetch REST config (flat)
DataSvc->>Lib: unflattenObject(flat)
DataSvc->>Lib: deepmerge(configurationDefaults, unflattenedConfig)
end
DataSvc-->>App: ConfigurationJson
App->>Provider: render Provider(config)
Provider->>UI: context exposes config
UI->>UI: read feature flags (announcement, tomorrow)
UI-->>Browser: render conditional Announcement and Tomorrow column
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 1
🧹 Nitpick comments (5)
components/Admin/Announcement/AddAnnouncement.tsx (1)
3-3: Consider usingimport typefor type-only imports.
AnnouncementDatais only used as a type annotation in this file. Usingimport typeis more explicit and can help bundlers with tree-shaking.Suggested change
-import { AnnouncementData } from "@/types/AnnouncementType" +import type { AnnouncementData } from "@/types/AnnouncementType"package.json (1)
50-50: Consider adding a format script for Prettier.Prettier is added as a dev dependency, but there's no corresponding npm script to run it. Consider adding format scripts to enable consistent usage across the team.
Suggested addition to scripts
"scripts": { "dev": "next dev", "build": "next build", "start": "next start", "lint": "next lint", - "test": "jest" + "test": "jest", + "format": "prettier --write .", + "format:check": "prettier --check ." },hooks/useConfiguration.ts (1)
8-10: Unnecessary null check in useConfiguration.The
ConfigurationContextis created withconfigurationDefaultsas its default value (line 12 inproviders/ConfigurationProvider.tsx), soctxwill never benullorundefined. This check is redundant and may confuse developers about when this error could actually occur.🔎 Consider removing the redundant check
export function useConfiguration(): ConfigurationJson { const ctx = useContext(ConfigurationContext) - - if (!ctx) { - throw new Error('useConfiguration must be used within provider') - } - return ctx }config/ConfigurationDefaults.ts (1)
3-3: Minor style: Remove space before colon in type annotation.The space before the colon in the type annotation is inconsistent with standard TypeScript conventions.
🔎 Proposed fix
-export const configurationDefaults : ConfigurationJson = { +export const configurationDefaults: ConfigurationJson = {app/page.tsx (1)
58-60: Unnecessary formatting change.The multi-line formatting of
slides.push()doesn't provide meaningful improvement over the previous single-line version for such a simple statement.🔎 Consider keeping the more concise format
- slides.push( - <UpcomingPrayerDayTiles times={times} key={times.display_date} />, - ) + slides.push(<UpcomingPrayerDayTiles times={times} key={times.display_date} />)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
README.mdapp/page.tsxcomponents/Admin/Announcement/AddAnnouncement.tsxcomponents/PrayerTimes/PrayerTimes.tsxconfig/ConfigurationDefaults.tshooks/useConfiguration.tspackage.jsonproviders/ConfigurationProvider.tsxservices/GoogleSheetsService.tsservices/MosqueDataService.tstypes/ConfigurationType.d.tstypes/MosqueDataType.d.ts
🧰 Additional context used
🧬 Code graph analysis (6)
services/GoogleSheetsService.ts (1)
types/ConfigurationType.d.ts (1)
ConfigurationJson(6-16)
hooks/useConfiguration.ts (2)
types/ConfigurationType.d.ts (1)
ConfigurationJson(6-16)providers/ConfigurationProvider.tsx (1)
ConfigurationContext(12-12)
providers/ConfigurationProvider.tsx (2)
types/ConfigurationType.d.ts (1)
ConfigurationJson(6-16)config/ConfigurationDefaults.ts (1)
configurationDefaults(3-13)
config/ConfigurationDefaults.ts (1)
types/ConfigurationType.d.ts (1)
ConfigurationJson(6-16)
components/PrayerTimes/PrayerTimes.tsx (2)
hooks/useConfiguration.ts (1)
useConfiguration(5-13)services/PrayerTimeService.ts (1)
getNextPrayer(30-56)
types/ConfigurationType.d.ts (1)
types/AnnouncementType.d.ts (1)
AnnouncementData(1-10)
🔇 Additional comments (15)
services/GoogleSheetsService.ts (2)
11-11: LGTM!The import of
ConfigurationJsonaligns with the new centralized type definition and ensures consistency across the configuration data flow.
90-90: Type alignment looks good.The parameter type change to
ConfigurationJsonis consistent with the return type ofsheetsGetConfigurationData()and the new type system introduced in this PR.package.json (1)
21-21: Good choice for configuration merging.
deepmergeis a well-maintained library suitable for deep merging configuration defaults with fetched data. The usage inMosqueDataService.getConfiguration()appropriately handles nested feature flags.components/PrayerTimes/PrayerTimes.tsx (3)
44-46: Clean feature flag implementation.The configuration hook usage and feature flag extraction are implemented correctly. The conditional rendering ensures the Tomorrow column is only shown when the feature is enabled.
67-67: LGTM!Conditionally rendering the header maintains proper table structure when the feature is toggled.
112-125: LGTM!The conditional rendering of the Tomorrow data cells is consistent with the header rendering logic.
types/MosqueDataType.d.ts (1)
3-3: Good type centralization.Importing
ConfigurationJsonfrom the centralized type definition eliminates duplication and ensures consistency across the codebase.services/MosqueDataService.ts (3)
17-21: Good setup for configuration merging.The imports for
deepmerge,ConfigurationJson, andconfigurationDefaultsestablish the foundation for the merged configuration pattern.
131-134: Well-implemented configuration merging.Using
deepmergeto combine defaults with fetched configuration ensures feature flags always have defined values, even when the API returns partial data. The fallback to an empty object handles the case whenconfigurationisundefined.
136-155: Clean refactoring ofgetAnnouncement.Deriving announcement data from the merged configuration ensures consistent behavior with the new configuration pipeline. The null checks and visibility logic remain intact.
types/ConfigurationType.d.ts (1)
1-16: Well-structured configuration type.The interface cleanly separates the announcement data from feature flags. The comment about syncing with
ConfigurationDefaults.tsis helpful, and the synchronization is already enforced at compile time—configurationDefaultsis explicitly typed asConfigurationJsoninconfig/ConfigurationDefaults.ts, ensuring TypeScript catches any structural mismatches.providers/ConfigurationProvider.tsx (1)
1-24: LGTM! Clean provider implementation.The
ConfigurationProvideris well-structured with proper typing and follows React Context best practices. The "use client" directive is appropriate for Next.js 15, and the provider correctly initializes the context with defaults while accepting runtime configuration.app/page.tsx (3)
16-16: LGTM! Configuration properly integrated into server component.The configuration is fetched at the server component level and correctly typed, which is appropriate for Next.js 15's architecture.
Also applies to: 28-29, 45-45
64-64: Excellent placement of ConfigurationProvider.Wrapping the entire page at the root level ensures all child components have access to the configuration context via the
useConfigurationhook.Also applies to: 94-94
91-91: Feature flag implementation looks correct.The conditional rendering of
Announcementbased onconfiguration.feature.announcement.enabledproperly implements the feature toggle pattern described in the PR objectives.
|
|
||
| - Google Account | ||
| - Google Cloud Project (Optional for Admin interface) | ||
| - Google Cloud Project |
There was a problem hiding this comment.
Documentation inconsistency between prerequisite sections.
This change correctly makes Google Cloud Project mandatory for the Admin Interface Setup. However, there's an inconsistency with line 60 which still shows "Google Cloud Project (Optional for Admin interface)" in the general prerequisites.
Consider updating line 60 as well for consistency, or clarifying that it's only optional if Admin Interface is not needed.
Suggested update for line 60
- Google Account
-- Google Cloud Project (Optional for Admin interface)
+- Google Cloud Project (Required for Admin interface only)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In README.md around lines 124 and 60, there is an inconsistency: line 124
correctly makes "Google Cloud Project" mandatory for Admin Interface Setup but
line 60 still lists "Google Cloud Project (Optional for Admin interface)".
Update line 60 to match the new requirement (e.g., remove "Optional for Admin
interface" and mark it as required) or add a clarifying note stating it's
optional only when the Admin Interface is not needed; ensure both places use
consistent wording and intent.
…et iframe to be generated via the admin interface
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
app/widget/embed/today-prayer-times/page.tsx (2)
5-5: Replaceanywith a specific type.The
searchParamstype should be more specific to maintain type safety. Define the expected query parameters.🔎 Proposed fix
-type Props = { searchParams: Promise<any> } +type Props = { + searchParams: Promise<{ + format?: string + sunrise?: string + date?: string + hijri?: string + }> +}
8-8: Fix spacing in destructuring.There's inconsistent spacing in the destructuring assignment.
🔎 Proposed fix
- const {format, sunrise, date, hijri } = await searchParams + const { format, sunrise, date, hijri } = await searchParamscomponents/Admin/Embed/EmbedTodayPrayerTimes/EmbedTodayPrayerTimes.tsx (1)
47-53: Add error handling for clipboard operations.The Clipboard API (
window.navigator.clipboard.writeText) may fail due to browser permissions, lack of HTTPS context, or browser compatibility. The operation should handle failures gracefully.🔎 Proposed enhancement with error handling
function copyUrlToClipboard() { - window.navigator.clipboard.writeText(widgetUrl) - setHasCopiedToClipboard(true) - setTimeout(() => { - setHasCopiedToClipboard(false) - }, 3000) + window.navigator.clipboard + .writeText(widgetUrl) + .then(() => { + setHasCopiedToClipboard(true) + setTimeout(() => { + setHasCopiedToClipboard(false) + }, 3000) + }) + .catch((err) => { + console.error('Failed to copy to clipboard:', err) + // Optionally show user-facing error message + }) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
app/widget/embed/today-prayer-times/TodayPrayerTime.tsxapp/widget/embed/today-prayer-times/page.tsxcomponents/Admin/AdminPage.tsxcomponents/Admin/Announcement/AddAnnouncement.tsxcomponents/Admin/Announcement/AnnoucementForm.tsxcomponents/Admin/Embed/EmbedTodayPrayerTimes/EmbedTodayPrayerTimes.tsxcomponents/ui/LoadingSpinner.tsxcomponents/ui/select.tsxcomponents/ui/spinner.tsxpackage.json
💤 Files with no reviewable changes (1)
- components/ui/LoadingSpinner.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (5)
components/ui/spinner.tsx (1)
lib/utils.ts (1)
cn(4-6)
app/widget/embed/today-prayer-times/TodayPrayerTime.tsx (4)
types/DailyPrayerTimeType.d.ts (1)
DailyPrayerTime(1-11)services/MosqueDataService.ts (1)
getPrayerTimesForToday(49-53)services/PrayerTimeService.ts (1)
getNextPrayer(30-56)lib/utils.ts (1)
cn(4-6)
app/widget/embed/today-prayer-times/page.tsx (1)
app/widget/embed/today-prayer-times/TodayPrayerTime.tsx (1)
TodayPrayerTime(15-157)
components/Admin/Announcement/AnnoucementForm.tsx (1)
components/ui/spinner.tsx (1)
Spinner(17-17)
components/ui/select.tsx (1)
lib/utils.ts (1)
cn(4-6)
🪛 ast-grep (0.40.3)
app/widget/embed/today-prayer-times/page.tsx
[warning] 30-30: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
components/Admin/Embed/EmbedTodayPrayerTimes/EmbedTodayPrayerTimes.tsx
[error] 17-17: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
app/widget/embed/today-prayer-times/page.tsx
[error] 31-31: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (7)
app/widget/embed/today-prayer-times/page.tsx (1)
30-44: Static analysis warning is a false positive.The
dangerouslySetInnerHTMLusage here is safe because the script content is entirely hardcoded with no user input. This is a standard pattern for enabling iframe auto-resize functionality by posting the document height to the parent window.app/widget/embed/today-prayer-times/TodayPrayerTime.tsx (1)
91-93: Nice date concatenation logic.The use of an array with conditional values,
filter(Boolean), andjoin()elegantly handles all combinations of showing the English date, Hijri date, or both.components/Admin/AdminPage.tsx (1)
4-5: LGTM!The integration of the
EmbedTodayPrayerTimescomponent is clean, and the flex layout adjustments appropriately accommodate multiple admin widgets.Also applies to: 29-31
components/Admin/Announcement/AnnoucementForm.tsx (1)
3-7: LGTM!The replacement of
LoadingSpinnerwith the newSpinnercomponent is clean and consistent. The label text formatting improvements enhance readability without changing functionality.Also applies to: 71-73, 111-113, 128-130, 155-158
components/Admin/Announcement/AddAnnouncement.tsx (1)
3-8: LGTM!The migration from
LoadingSpinnerto the newSpinnercomponent is consistent with the PR's refactoring goals. Layout refinements and formatting changes improve code consistency.Also applies to: 36-50
components/Admin/Embed/EmbedTodayPrayerTimes/EmbedTodayPrayerTimes.tsx (1)
57-194: Well-structured embed widget component.The component provides a clean UI for configuring and previewing the embeddable widget, with good UX touches like the loading overlay, copy-to-clipboard functionality, and visual feedback. The use of controlled inputs and the Select component is appropriate.
components/ui/select.tsx (1)
1-159: LGTM! Well-structured Radix UI Select wrapper.This component follows best practices for wrapping Radix UI primitives:
- Proper use of
forwardReffor all componentsdisplayNameset for DevTools support- Consistent styling via the
cnutility- Accessibility handled by underlying Radix primitives
- Clean separation of concerns with individual exported components
| import { CheckIcon } from "lucide-react" | ||
| import { cn } from "@/lib/utils" | ||
|
|
||
| export default function EmbedTodayPrayerTimes({}) { |
There was a problem hiding this comment.
Remove the empty object pattern from function parameters.
The empty object pattern {} serves no purpose here and is flagged by Biome as a correctness issue.
🔎 Proposed fix
-export default function EmbedTodayPrayerTimes({}) {
+export default function EmbedTodayPrayerTimes() {📝 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.
| export default function EmbedTodayPrayerTimes({}) { | |
| export default function EmbedTodayPrayerTimes() { |
🧰 Tools
🪛 Biome (2.1.2)
[error] 17-17: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
🤖 Prompt for AI Agents
In components/Admin/Embed/EmbedTodayPrayerTimes/EmbedTodayPrayerTimes.tsx around
line 17, the function signature currently uses an unnecessary empty object
pattern ({}) as parameters; remove the empty pattern and change the signature to
either no parameters (function EmbedTodayPrayerTimes()) or a typed props
parameter if this component should accept props (e.g., function
EmbedTodayPrayerTimes(props: PropsType)), and update any internal references or
usages accordingly.
| useEffect(() => { | ||
| setIsIFrameLoading(true) | ||
| const url = `${window.location.origin}/widget/embed/today-prayer-times?format=${format}&sunrise=${sunrise}&date=${date}&hijri=${hijri}` | ||
| setWidgetUrl(url) | ||
| setWidgetIFrameScriptStr(` | ||
| <iframe | ||
| src="${url}" | ||
| width="100%" | ||
| height="auto" | ||
| frameborder="0" | ||
| style="border: none" | ||
| ></iframe> | ||
| `) | ||
| }, [format, sunrise, date, hijri]) |
There was a problem hiding this comment.
Guard against SSR failure when accessing window.location.
The useEffect accesses window.location.origin directly (line 30), which will throw a ReferenceError during server-side rendering since window is undefined on the server. Although this component is marked "use client", defensive coding is recommended.
🔎 Proposed fix with SSR guard
useEffect(() => {
setIsIFrameLoading(true)
- const url = `${window.location.origin}/widget/embed/today-prayer-times?format=${format}&sunrise=${sunrise}&date=${date}&hijri=${hijri}`
+ const origin = typeof window !== 'undefined' ? window.location.origin : ''
+ const url = `${origin}/widget/embed/today-prayer-times?format=${format}&sunrise=${sunrise}&date=${date}&hijri=${hijri}`
setWidgetUrl(url)
setWidgetIFrameScriptStr(`
<iframe
src="${url}"
width="100%"
height="auto"
frameborder="0"
style="border: none"
></iframe>
`)
}, [format, sunrise, date, hijri])🤖 Prompt for AI Agents
In components/Admin/Embed/EmbedTodayPrayerTimes/EmbedTodayPrayerTimes.tsx around
lines 28 to 41, the useEffect reads window.location.origin directly which will
throw during SSR; guard against this by checking typeof window !== "undefined"
(or computing const origin = typeof window !== "undefined" ?
window.location.origin : "") before building the URL and only call
setWidgetUrl/setWidgetIFrameScriptStr when an origin is available (or use a safe
fallback), so the effect is no-op on the server and only constructs the iframe
string on the client.
| // @ts-ignore | ||
| <Loader2Icon | ||
| role="status" | ||
| aria-label="Loading" | ||
| className={cn("size-4 animate-spin", className)} | ||
| {...props} | ||
| /> |
There was a problem hiding this comment.
Remove the @ts-ignore directive and fix the underlying type issue.
The @ts-ignore comment suppresses TypeScript errors, which is problematic for type safety. The issue likely stems from Loader2Icon expecting specific props that differ from standard SVG element props.
🔎 Investigate and fix the type issue
The proper solution depends on the actual type error. Common fixes include:
- If
Loader2Iconaccepts its own props, explicitly type them:
-function Spinner({ className, ...props }: React.ComponentProps<"svg">) {
+function Spinner({ className, ...props }: React.ComponentProps<typeof Loader2Icon>) {
return (
- // @ts-ignore
<Loader2Icon
role="status"
aria-label="Loading"
className={cn("size-4 animate-spin", className)}
{...props}
/>
)
}- If there's a prop mismatch, selectively spread only compatible props or cast appropriately.
Run this script to check the Loader2Icon type definition:
#!/bin/bash
# Search for Loader2Icon type definition or usage patterns
rg -n "Loader2Icon" -A 3 -B 3 --type=ts --type=tsxCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In components/ui/spinner.tsx around lines 7-13, remove the // @ts-ignore and fix
the underlying prop typing: identify Loader2Icon's declared props (or its
library's types) and ensure the props you spread are compatible by typing the
component props as React.SVGProps<SVGSVGElement> (or the specific
Loader2IconProps) and/or explicitly casting the spread to that SVG prop type;
alternatively restrict the spread to only known-compatible props (className,
role, aria-label) and pass other props via a typed rest variable so TypeScript
can validate compatibility instead of ignoring the error.
…, calendar prints
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
README.md (1)
76-76: Resolve Google Cloud Project prerequisite inconsistency.Line 76 states "Google Cloud Project (Optional for Admin interface)" while line 140 (Admin Interface Setup prerequisites) lists it as required. This inconsistency remains from a previous review comment. Clarify whether Google Cloud Project is truly optional (update line 140) or required (update line 76).
Suggested clarification
If Google Cloud Project is required for Admin interface, update line 76 to:
- Google Cloud Project (Optional for Admin interface) + Google Cloud Project (Required for Admin interface)Or if it's only required when using Admin interface, add a clarifying note like: "Google Cloud Project (Optional, required only if using Admin interface)"
Also applies to: 140-140
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
public/screenshots/demo-admin-mobile-view.pngis excluded by!**/*.pngpublic/screenshots/demo-admin-view.pngis excluded by!**/*.pngpublic/screenshots/demo-mobile-view.pngis excluded by!**/*.pngpublic/screenshots/demo-mosque-announcement-car.pngis excluded by!**/*.pngpublic/screenshots/demo-mosque-calendar-prints-1.pngis excluded by!**/*.pngpublic/screenshots/demo-mosque-calendar-prints-2.pngis excluded by!**/*.pngpublic/screenshots/demo-mosque-calendar-prints.pngis excluded by!**/*.pngpublic/screenshots/demo-mosque-view-1.pngis excluded by!**/*.pngpublic/screenshots/demo-mosque-view-2.pngis excluded by!**/*.pngpublic/screenshots/demo-mosque-view-3.pngis excluded by!**/*.png
📒 Files selected for processing (1)
README.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
47-47: Images should have alternate text (alt text)
(MD045, no-alt-text)
49-49: Images should have alternate text (alt text)
(MD045, no-alt-text)
52-52: Images should have alternate text (alt text)
(MD045, no-alt-text)
56-56: Images should have alternate text (alt text)
(MD045, no-alt-text)
60-60: Images should have alternate text (alt text)
(MD045, no-alt-text)
61-61: Images should have alternate text (alt text)
(MD045, no-alt-text)
65-65: Images should have alternate text (alt text)
(MD045, no-alt-text)
69-69: Images should have alternate text (alt text)
(MD045, no-alt-text)
| ### Mosque views | ||
|
|
||
| <img src="./public/demo-mosque-view-1.png" /> | ||
| <img src="./public/screenshots/demo-mosque-view-1.png" /> |
There was a problem hiding this comment.
Add alt text to all images for accessibility.
All 8 images are missing alt text attributes, which is both an accessibility compliance issue (WCAG) and a markdown lint violation flagged by markdownlint-cli2 (MD045). Provide descriptive alt text that conveys the purpose of each screenshot.
🔎 Proposed fixes to add alt text
-<img src="./public/screenshots/demo-mosque-view-1.png" />
+<img src="./public/screenshots/demo-mosque-view-1.png" alt="Mosque prayer display screen showing prayer times in a mosque setting" />
-<img src="./public/screenshots/demo-mosque-view-2.png" />
+<img src="./public/screenshots/demo-mosque-view-2.png" alt="Alternative mosque prayer display screen layout" />
-<img src="public/screenshots/demo-mosque-view-3.png" />
+<img src="public/screenshots/demo-mosque-view-3.png" alt="Mosque prayer display screen with tomorrow prayer times hidden" />
-<img src="./public/screenshots/demo-mosque-announcement-car.png" />
+<img src="./public/screenshots/demo-mosque-announcement-car.png" alt="Announcement feature displayed on mosque screen" />
-<img src="public/screenshots/demo-mosque-calendar-prints-1.png" />
+<img src="public/screenshots/demo-mosque-calendar-prints-1.png" alt="Printable prayer calendar example 1" />
-<img src="public/screenshots/demo-mosque-calendar-prints-2.png" />
+<img src="public/screenshots/demo-mosque-calendar-prints-2.png" alt="Printable prayer calendar example 2" />
-<img src="./public/screenshots/demo-mobile-view.png" width="500px" />
+<img src="./public/screenshots/demo-mobile-view.png" width="500px" alt="Mobile app view of prayer times" />
-<img src="./public/screenshots/demo-admin-view.png/"/>
+<img src="./public/screenshots/demo-admin-view.png" alt="Admin interface for managing mosque screen" />Also applies to: 49-49, 52-52, 56-56, 60-60, 61-61, 65-65, 69-69
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
47-47: Images should have alternate text (alt text)
(MD045, no-alt-text)
🤖 Prompt for AI Agents
In README.md around line 47 (and also address lines 49, 52, 56, 60, 61, 65, 69),
the <img> tags lack alt attributes; add concise, descriptive alt text for each
screenshot that conveys the purpose/content of the image (e.g., "Mosque view -
map and details", "Search results - filters applied", etc.), updating each <img
src="..."> to include an appropriate alt="..." attribute so images are
accessible and satisfy markdownlint (MD045) and WCAG requirements.
…ride the property in the sheets, seems to ignore nulls
…ouncement fetch to 60s to keep a buffer
…nt configuration -> config - this change includes some refactoring to announcement to set a variable font size based on height as the screens are usually very wide and we already have a max width set - add GoogleSheetsService.ts functions to fetch mosque data, this will allow users to bypass the mosque tech api endpoint and fetch data directly from sheets api - there is a 300 request per second read limit on sheets api by default
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 (2)
services/GoogleSheetsService.ts (1)
162-172: Potential null pointer dereference onannouncement.At line 164,
announcementcould benull(whendata?.announcementis undefined). However, lines 167-171 accessannouncement.is_visibleandannouncement?.datewithout first checking ifannouncementis null. This will throw a runtime error.🔎 Proposed fix
export async function sheetsGetAnnouncement(): Promise<AnnouncementData | null> { const data = await sheetsGetConfigurationData() let announcement = data?.announcement as unknown as AnnouncementData ?? null + if (!announcement) { + return null + } + const now = moment() announcement.is_visible = ( now.isSame(announcement?.date, 'day') && now.isSameOrAfter(`${announcement?.date} ${announcement?.start_time}`, 'minutes') && now.isBefore(`${announcement?.date} ${announcement?.end_time}`, 'minutes') ) return announcement }app/page.tsx (1)
40-47: Add error handling for async data fetches in the Home component.The Home component calls
getConfiguration(),getPrayerTimesForToday(),getPrayerTimesForTomorrow(),getJummahTimes(),getMetaData(), andgetPrayerTimesForUpcomingDays()without error handling. If any of these throw, the page will fail to render. Consider wrapping these calls in try-catch blocks or creating anerror.tsxfile to define an error boundary for this route.
🧹 Nitpick comments (7)
components/Announcement/Announcement.tsx (1)
11-20: Consider using async/await and handling errors more gracefully.The
fetchAnnouncementfunction silently logs errors without updating UI state. If the fetch fails, users won't know there's an issue. Consider adding an error state or retry logic.Also, the function is defined inside the component but doesn't depend on any props/state, so it could be defined outside or wrapped in
useCallbackfor clarity (though not strictly necessary here sincesetNextAnnouncementis stable).🔎 Proposed refactor using async/await
- function fetchAnnouncement () { - fetch(`/api/data/announcements?no_cache=${Date.now()}`) - .then((response) => response.json()) - .then(({ announcement }) => { - setNextAnnouncement(announcement) - }) - .catch((error) => { - console.log(`error fetching announcements: ${error}`) - }) - } + async function fetchAnnouncement() { + try { + const response = await fetch(`/api/data/announcements?no_cache=${Date.now()}`) + const { announcement } = await response.json() + setNextAnnouncement(announcement) + } catch (error) { + console.error(`Error fetching announcements: ${error}`) + } + }services/GoogleSheetsUtil.ts (2)
6-8: Simplify the double-negation pattern.The expression
!(!A || !B)is logically equivalent toA && Band is easier to read.🔎 Proposed simplification
export function isSheetsClientEnabled(): boolean { - return !(!ADMIN_GOOGLE_SA_EMAIL || !ADMIN_GOOGLE_SA_PRIVATE_KEY) + return Boolean(ADMIN_GOOGLE_SA_EMAIL && ADMIN_GOOGLE_SA_PRIVATE_KEY) }
122-123: Avoid@ts-ignoreby using proper typing.The
@ts-ignoresuppresses type checking. Consider using aPartial<DailyPrayerTime>or a builder pattern to construct the object incrementally.🔎 Proposed fix
return rows.map((row) => { - //@ts-ignore - const obj: any = { } + const obj: Partial<DailyPrayerTime> & Record<string, any> = {}services/GoogleSheetsService.ts (1)
54-56: Duplicate spreadsheet access check.The spreadsheet access check in
getUserSheetsClient(lines 54-56) duplicates the check inisSheetsClientReady(lines 67-69). Consider whether this validation belongs only inisSheetsClientReadyor ifgetUserSheetsClientshould just return the client without the extra API call.services/MosqueDataService.ts (2)
43-44: Fix the comment grammar and clarify intent.The comment "we do this so that the MosqueData type doesn't is strongly typed" is grammatically incorrect and unclear.
🔎 Proposed fix
- // we do this so that the MosqueData type doesn't is strongly typed - data.config = unflattenObject(data.config) + // Unflatten the config object to match the ConfigurationJson structure + data.config = unflattenObject(data.config ?? {})
149-149: Consider removing or lowering log level for configuration output.
console.logwith the full configuration JSON will be noisy in production logs. Consider usingconsole.debugor removing this once the feature is stable.🔎 Proposed fix
- console.log(`configuration: ${JSON.stringify(mergedConfig)}`) + // Remove or use console.debug in production + console.debug(`configuration: ${JSON.stringify(mergedConfig)}`)app/page.tsx (1)
63-65: Remove debug console.log before production.This console.log outputs configuration state on every page load. Consider removing it or using a more appropriate logging level.
🔎 Proposed fix
- console.log( - `configuration.feature.announcement.enabled: ${config.feature.announcement.enabled}`, - )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/page.tsxcomponents/Admin/Announcement/AnnoucementForm.tsxcomponents/Announcement/Announcement.tsxcomponents/PrayerTimes/PrayerTimes.tsxhooks/useConfiguration.tslib/unflattenObject.tsproviders/ConfigurationProvider.tsxservices/GoogleSheetsService.tsservices/GoogleSheetsUtil.tsservices/MosqueDataService.tstypes/MosqueDataType.d.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- components/PrayerTimes/PrayerTimes.tsx
- providers/ConfigurationProvider.tsx
- hooks/useConfiguration.ts
- components/Admin/Announcement/AnnoucementForm.tsx
- types/MosqueDataType.d.ts
🧰 Additional context used
🧬 Code graph analysis (5)
app/page.tsx (3)
types/ConfigurationType.d.ts (1)
ConfigurationJson(6-16)services/MosqueDataService.ts (1)
getConfiguration(146-151)providers/ConfigurationProvider.tsx (1)
ConfigurationProvider(14-24)
services/GoogleSheetsUtil.ts (1)
types/DailyPrayerTimeType.d.ts (1)
DailyPrayerTime(1-11)
components/Announcement/Announcement.tsx (2)
types/AnnouncementType.d.ts (1)
AnnouncementData(1-10)components/Announcement/AnnouncementModal.tsx (1)
AnnouncementModal(8-30)
services/GoogleSheetsService.ts (6)
types/MosqueDataType.d.ts (2)
MosqueData(5-10)MosqueMetadataType(14-14)config/ConfigurationDefaults.ts (1)
configurationDefaults(3-13)types/DailyPrayerTimeType.d.ts (1)
DailyPrayerTime(1-11)services/GoogleSheetsUtil.ts (4)
prayerTimeValuesToPrayerTimesJsonSchema(115-164)sheetsUtilValuesToJson(15-30)sheetsUtilValuesToNestedJson(32-66)sheetsUtilFlattenedJsonToRows(91-108)types/JummahTimesType.d.ts (1)
JummahTimes(1-1)types/ConfigurationType.d.ts (1)
ConfigurationJson(6-16)
services/MosqueDataService.ts (6)
services/GoogleSheetsUtil.ts (1)
isSheetsClientEnabled(6-8)types/MosqueDataType.d.ts (2)
MosqueData(5-10)MosqueMetadataType(14-14)services/GoogleSheetsService.ts (1)
sheetsGetMosqueData(77-98)lib/unflattenObject.ts (1)
unflattenObject(12-32)types/ConfigurationType.d.ts (1)
ConfigurationJson(6-16)config/ConfigurationDefaults.ts (1)
configurationDefaults(3-13)
🔇 Additional comments (8)
components/Announcement/Announcement.tsx (2)
22-28: LGTM!The 60-second interval aligns with the PR objective to stay within Google Sheets read limits. The immediate fetch on mount and proper cleanup of the interval are correctly implemented.
41-42: LGTM!The null check combined with the length check correctly handles cases where
car_reg_numbermight be an empty string, ensuring the car registration view only displays when there's actual content.lib/unflattenObject.ts (2)
4-10: LGTM! Parsing logic is sound.The
parseValuefunction correctly handles boolean strings and numeric conversion. The check!isNaN(Number(value))properly rejects strings like"123abc"sinceNumber("123abc")returnsNaN. The empty string check prevents""from being converted to0.
12-31: LGTM!The
unflattenObjectfunction correctly builds nested structures from dot-delimited keys. The use of??=for lazy object creation is idiomatic and clean.Note: This implementation does not support array notation (e.g.,
items.0.name), but this appears to be intentional based on the current use case with configuration objects.services/GoogleSheetsService.ts (1)
77-98: LGTM!The
sheetsGetMosqueDatafunction properly aggregates data from multiple sheets and provides sensible defaults on error. The fallback toconfigurationDefaultsensures the app remains functional even when sheets access fails.services/MosqueDataService.ts (2)
29-29: Module-level evaluation ofisSheetsClientEnabled().
useSheetsClientis evaluated once at module load time. If environment variables change, a server restart is required. This is likely intentional for performance, but worth documenting.
153-172: LGTM!The
getAnnouncementfunction properly checks for nullannouncement.dateat line 157 before proceeding with visibility calculations. The moment-based time comparison logic is correct.app/page.tsx (1)
68-98: LGTM!The
ConfigurationProvidercorrectly wraps the page content, making configuration available to child components. The conditional rendering ofAnnouncementbased onconfig.feature.announcement.enabledis clean and aligns with the PR objective.
| // Special case for ASR (first & second) | ||
| if (header.startsWith('asr_first_')) { | ||
| obj.asr = obj.asr || {} | ||
| obj.asr.start = value | ||
| return | ||
| } | ||
|
|
||
| if (header.startsWith('asr_second_')) { | ||
| obj.asr = obj.asr || {} | ||
| obj.asr.start_secondary = value | ||
| return | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for PrayerTime interface definition
find . -type f -name "*.ts" -o -name "*.tsx" | xargs grep -l "PrayerTime" | head -20Repository: MosqueOS/Mosque-Prayer-Display-Screen
Length of output: 955
🏁 Script executed:
# Search for any congregation-related patterns
rg -n "congregation" --type=ts -A 2 -B 2Repository: MosqueOS/Mosque-Prayer-Display-Screen
Length of output: 21495
🏁 Script executed:
# Search for asr_ headers and patterns
rg -n "asr_" --type=ts -A 2 -B 2Repository: MosqueOS/Mosque-Prayer-Display-Screen
Length of output: 695
🏁 Script executed:
# Get full context of GoogleSheetsUtil.ts around the reviewed lines
find . -type f -name "GoogleSheetsUtil.ts" -exec wc -l {} \;Repository: MosqueOS/Mosque-Prayer-Display-Screen
Length of output: 115
🏁 Script executed:
sed -n '130,165p' services/GoogleSheetsUtil.tsRepository: MosqueOS/Mosque-Prayer-Display-Screen
Length of output: 924
Fix ASR special case handlers to preserve field names.
The asr_first_ and asr_second_ special cases incorrectly hardcode mapping to start and start_secondary. This causes headers like asr_first_congregation_start to only set start, discarding the congregation_start part. Since congregation_start is a required field on PrayerTime (used throughout the app in PrayerTimeService, UI rendering, etc.), these headers would result in missing congregation time data. Extract and preserve the actual field name suffix: replace the hardcoded assignments with dynamic key assignment like the generic fallback at lines 156-159.
🤖 Prompt for AI Agents
In services/GoogleSheetsUtil.ts around lines 140 to 151, the ASR special-case
branches hardcode keys ("start" and "start_secondary") which drops the actual
suffix (e.g., "congregation_start"); change both branches to extract the suffix
after the prefix (remove "asr_first_" or "asr_second_"), normalize it (trim
leading underscores if any), ensure obj.asr exists, and assign value dynamically
as obj.asr[suffix] = value so the field name is preserved (same approach as the
generic fallback at lines ~156-159).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
README.md (3)
47-49: Add alt text to all images for accessibility and compliance.All 8 images are missing alt text attributes, which violates WCAG accessibility requirements and markdownlint (MD045). This was previously flagged and remains unaddressed.
🔎 Proposed fixes to add alt text
-<img src="./public/screenshots/demo-mosque-view-1.png" /> +<img src="./public/screenshots/demo-mosque-view-1.png" alt="Mosque prayer display showing today's prayer times" /> -<img src="./public/screenshots/demo-mosque-view-2.png" /> +<img src="./public/screenshots/demo-mosque-view-2.png" alt="Alternative mosque prayer display layout" /> -<img src="./public/screenshots/demo-mosque-view-3.png" /> +<img src="./public/screenshots/demo-mosque-view-3.png" alt="Mosque prayer display with tomorrow prayer times hidden" /> -<img src="./public/screenshots/demo-mosque-announcement-car.png" /> +<img src="./public/screenshots/demo-mosque-announcement-car.png" alt="Announcement feature displayed on mosque screen" /> -<img src="./public/screenshots/demo-mosque-calendar-prints-1.png" /> +<img src="./public/screenshots/demo-mosque-calendar-prints-1.png" alt="Printable prayer calendar example" /> -<img src="./public/screenshots/demo-mosque-calendar-prints-2.png" /> +<img src="./public/screenshots/demo-mosque-calendar-prints-2.png" alt="Printable prayer calendar second example" /> -<img src="./public/screenshots/demo-mobile-view.png" width="500px" /> +<img src="./public/screenshots/demo-mobile-view.png" width="500px" alt="Mobile app view of prayer times" /> -<img src="./public/screenshots/demo-admin-view.png/"/> +<img src="./public/screenshots/demo-admin-view.png" alt="Admin interface for managing mosque screen" />Also applies to: 52-52, 56-56, 60-61, 65-65, 69-69
69-69: Remove invalid trailing slash from image path.Line 69 has an invalid trailing slash in the filename. This was flagged previously and remains unresolved.
-<img src="./public/screenshots/demo-admin-view.png/"/> +<img src="./public/screenshots/demo-admin-view.png" alt="..." />
76-76: Fix inconsistency between general and Admin Interface prerequisites.Line 76 states "Google Cloud Project (Optional for Admin interface)" while line 140 now correctly marks it as required (removed "Optional"). This PR changes line 140 but not line 76, creating documentation inconsistency. This issue was previously flagged and persists.
- - Google Cloud Project (Optional for Admin interface) + - Google Cloud Project (Required for Admin interface)Also applies to: 140-140
🧹 Nitpick comments (1)
services/GoogleSheetsService.ts (1)
54-58: Consider removing the redundant test API call.Every function that calls
getUserSheetsClient()immediately makes its own API request (e.g.,sheetsGetPrayerDatacallssheets.spreadsheets.values.get). The test call at lines 54-56 doubles the API requests without adding value—if credentials are invalid, the subsequent call will fail anyway with a clear error.🔎 Proposed refactor
- const sheets = google.sheets({ - version: "v4", - auth: googleAuthJwt, - }) - - await sheets.spreadsheets.get({ - spreadsheetId: SPREADSHEET_ID, - }) - - return sheets + return google.sheets({ + version: "v4", + auth: googleAuthJwt, + })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.mdservices/GoogleSheetsService.ts
🧰 Additional context used
🧬 Code graph analysis (1)
services/GoogleSheetsService.ts (5)
types/MosqueDataType.d.ts (2)
MosqueData(5-10)MosqueMetadataType(14-14)config/ConfigurationDefaults.ts (1)
configurationDefaults(3-13)services/GoogleSheetsUtil.ts (4)
prayerTimeValuesToPrayerTimesJsonSchema(115-164)sheetsUtilValuesToJson(15-30)sheetsUtilValuesToNestedJson(32-66)sheetsUtilFlattenedJsonToRows(91-108)types/JummahTimesType.d.ts (1)
JummahTimes(1-1)types/ConfigurationType.d.ts (1)
ConfigurationJson(6-16)
🪛 markdownlint-cli2 (0.18.1)
README.md
47-47: Images should have alternate text (alt text)
(MD045, no-alt-text)
49-49: Images should have alternate text (alt text)
(MD045, no-alt-text)
52-52: Images should have alternate text (alt text)
(MD045, no-alt-text)
56-56: Images should have alternate text (alt text)
(MD045, no-alt-text)
60-60: Images should have alternate text (alt text)
(MD045, no-alt-text)
61-61: Images should have alternate text (alt text)
(MD045, no-alt-text)
65-65: Images should have alternate text (alt text)
(MD045, no-alt-text)
69-69: Images should have alternate text (alt text)
(MD045, no-alt-text)
🔇 Additional comments (4)
services/GoogleSheetsService.ts (4)
24-29: LGTM! Centralizing sheet names improves maintainability.The
SHEET_NAMESconstant provides a single source of truth for sheet references, reducing the risk of typos and making future updates easier.
77-98: Verify the silent error-handling behavior is intentional.The catch block returns fallback data without propagating errors to the caller. While this provides resilience, it means:
- Callers cannot distinguish between legitimate empty data and a failed API call
- Configuration or credential issues may go unnoticed in production
- The specific failing sub-call is unclear from the logged error
Consider whether callers need to know about failures, or if returning partial data (e.g., successful fetches with defaults for failed ones) would be more appropriate.
100-146: LGTM! Data-fetch functions are well-structured.The three helper functions follow a consistent pattern:
- Appropriate error handling with descriptive messages
- Correct use of transformation utilities
- Proper type casting
155-158: Excellent! The deepmerge fix correctly addresses the previous critical issue.The code now properly converts the 2D array to a nested object using
sheetsUtilValuesToNestedJsonbefore passing it todeepmerge. This ensures both arguments are objects, which is whatdeepmergeexpects.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/page.tsx (1)
40-47: Consider parallelizing independent data fetches for better performance.The sequential
awaitcalls forgetPrayerTimesForToday(),getPrayerTimesForTomorrow(),getJummahTimes(),getMetaData(),getConfiguration(), andgetPrayerTimesForUpcomingDays()are independent and could be fetched concurrently usingPromise.all()to reduce overall page load time.🔎 Proposed optimization
export default async function Home() { - const today: DailyPrayerTime = await getPrayerTimesForToday() - const tomorrow: DailyPrayerTime = await getPrayerTimesForTomorrow() - const jummahTimes: JummahTimes = await getJummahTimes() - const mosqueMetadata: MosqueMetadataType = await getMetaData() - const config: ConfigurationJson = await getConfiguration() - const upcomingPrayerDays: UpcomingPrayerTimes[] = - await getPrayerTimesForUpcomingDays() + const [today, tomorrow, jummahTimes, mosqueMetadata, config, upcomingPrayerDays] = + await Promise.all([ + getPrayerTimesForToday(), + getPrayerTimesForTomorrow(), + getJummahTimes(), + getMetaData(), + getConfiguration(), + getPrayerTimesForUpcomingDays(), + ])
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/page.tsxcomponents/Admin/Announcement/AnnoucementForm.tsxcomponents/Announcement/Announcement.tsxservices/MosqueDataService.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- components/Admin/Announcement/AnnoucementForm.tsx
- components/Announcement/Announcement.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
app/page.tsx (4)
types/ConfigurationType.d.ts (1)
ConfigurationJson(6-16)services/MosqueDataService.ts (1)
getConfiguration(144-148)providers/ConfigurationProvider.tsx (1)
ConfigurationProvider(14-24)components/PrayerTimes/PrayerTimes.tsx (1)
PrayerTimes(9-131)
🔇 Additional comments (3)
app/page.tsx (1)
64-94: LGTM!The
ConfigurationProviderwrapper and conditionalAnnouncementrendering are well-implemented. The feature flag access path (config.feature.announcement.enabled) aligns with theConfigurationJsontype definition.services/MosqueDataService.ts (2)
144-148: LGTM!The configuration merging logic correctly handles null/undefined config by falling back to an empty object, ensuring defaults from
configurationDefaultsare always applied. This provides a robust configuration pipeline.
150-169: LGTM!The
getAnnouncementfunction correctly leverages the newgetConfiguration()to retrieve announcement data and properly handles visibility logic based on date/time comparisons.
| export async function getMosqueData (): Promise<MosqueData> { | ||
| if (useSheetsClient) { | ||
| return await sheetsGetMosqueData() | ||
| } else { | ||
| const response = await fetch(MOSQUE_API_ENDPOINT, { | ||
| next: { revalidate: 30 }, | ||
| }) | ||
|
|
||
| export async function getMosqueData(): Promise<MosqueData> { | ||
| const response = await fetch(MOSQUE_API_ENDPOINT, { | ||
| next: { revalidate: 30 }, | ||
| }) | ||
| const data = await response.json() | ||
|
|
||
| return response.json() | ||
| // we do this so that the MosqueData type doesn't is strongly typed | ||
| data.config = unflattenObject(data.config) | ||
|
|
||
| return data | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for API fetch failures.
The fetch call lacks response status validation and error handling. If the API returns a non-2xx status or the network request fails, this will result in an uncaught error or malformed data.
🔎 Proposed fix with error handling
export async function getMosqueData (): Promise<MosqueData> {
if (useSheetsClient) {
return await sheetsGetMosqueData()
} else {
const response = await fetch(MOSQUE_API_ENDPOINT, {
next: { revalidate: 30 },
})
+ if (!response.ok) {
+ throw new Error(`Failed to fetch mosque data: ${response.status} ${response.statusText}`)
+ }
+
const data = await response.json()
- // we do this so that the MosqueData type doesn't is strongly typed
+ // Unflatten the config object to match the MosqueData type structure
data.config = unflattenObject(data.config)
return data
}
}🤖 Prompt for AI Agents
In services/MosqueDataService.ts around lines 31 to 46, the fetch path lacks
response status validation and exception handling; wrap the
fetch/response.json() in a try/catch, check response.ok after the fetch and
throw or return a clear error when status is non-2xx, handle JSON parse errors,
and ensure any caught errors are logged (or rethrown) so callers don’t receive
malformed data; keep the sheetsClient branch unchanged and only add robust error
handling and clear error messages for the API branch.
| // we do this so that the MosqueData type doesn't is strongly typed | ||
| data.config = unflattenObject(data.config) |
There was a problem hiding this comment.
Typo in comment and potential null safety issue.
The comment on line 41 has a grammatical error. Additionally, data.config could be undefined from the API response; consider adding a guard.
🔎 Proposed fix
- // we do this so that the MosqueData type doesn't is strongly typed
- data.config = unflattenObject(data.config)
+ // Unflatten the config object to match the MosqueData type structure
+ data.config = data.config ? unflattenObject(data.config) : {}📝 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.
| // we do this so that the MosqueData type doesn't is strongly typed | |
| data.config = unflattenObject(data.config) | |
| // Unflatten the config object to match the MosqueData type structure | |
| data.config = data.config ? unflattenObject(data.config) : {} |
🤖 Prompt for AI Agents
In services/MosqueDataService.ts around lines 41-42, fix the comment grammar to
read something like "we do this so that the MosqueData type is strongly typed"
and add a null-safety guard before calling unflattenObject: only call
unflattenObject when data.config is defined and is an object, otherwise assign a
sensible default (e.g., an empty object) or leave it undefined according to the
intended type; ensure you cast/declare the value as a Record<string, any> (or
the appropriate type) so TypeScript stays happy.
…es and times are in arabic
…es and times are in arabic
…mit requests to the sheets api
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
components/PrayerTimes/PrayerTimes.tsx (1)
89-96: Missing locale enforcement forstart_secondary.The primary start time now uses
.locale("en"), butstart_secondaryon line 92-94 is missing the locale call, which could result in inconsistent formatting.🔎 Proposed fix
{prayer.data?.start_secondary ? ( <div className="block mt-1 md:mt-2"> - {moment(prayer.data.start_secondary, ["HH:mm"]).format( + {moment(prayer.data.start_secondary, ["HH:mm"]).locale("en").format( "h:mm", )} </div> ) : null}services/GoogleSheetsService.ts (1)
163-174: Add.locale("en")to moment instance on line 167.The codebase consistently enforces English locale on moment instances throughout client components (e.g.,
Date.tsx,Clock.tsx), but this service function creates a moment instance without it. Line 167 should be:const now = moment().locale("en")to ensure consistent locale enforcement across the application and maintain proper date/time handling for the visibility comparison logic.Additionally, the double type cast on line 165 (
as unknown as AnnouncementData) is a code smell. Consider fixing the type mismatch at the source.package.json (1)
16-17: Update React TypeScript types to match React version.When upgrading to Next.js 16, you should also upgrade @types/react and @types/react-dom to their latest versions. The current versions (18.2.21 and 18.2.7) are outdated.
🔎 Proposed fix
- "@types/react": "18.2.21", - "@types/react-dom": "18.2.7", + "@types/react": "^19.0.0", + "@types/react-dom": "^19.0.0",
♻️ Duplicate comments (2)
components/Admin/Embed/EmbedTodayPrayerTimes/EmbedTodayPrayerTimes.tsx (2)
17-17: Remove the empty object pattern from function parameters.The empty object pattern serves no purpose and is flagged as a correctness issue.
🔎 Proposed fix
-export default function EmbedTodayPrayerTimes({}) { +export default function EmbedTodayPrayerTimes() {
28-41: Guard against SSR failure when accessingwindow.location.The direct access to
window.location.originwill throw a ReferenceError during server-side rendering.🔎 Proposed fix with SSR guard
useEffect(() => { setIsIFrameLoading(true) - const url = `${window.location.origin}/widget/embed/today-prayer-times?format=${format}&sunrise=${sunrise}&date=${date}&hijri=${hijri}` + const origin = typeof window !== 'undefined' ? window.location.origin : '' + const url = `${origin}/widget/embed/today-prayer-times?format=${format}&sunrise=${sunrise}&date=${date}&hijri=${hijri}` setWidgetUrl(url) setWidgetIFrameScriptStr(` <iframe src="${url}" width="100%" height="auto" frameborder="0" style="border: none" ></iframe> `) }, [format, sunrise, date, hijri])
🧹 Nitpick comments (6)
components/Admin/Embed/EmbedTodayPrayerTimes/EmbedTodayPrayerTimes.tsx (2)
88-92: Remove commented code.The commented input element appears to be superseded by the Select component above. Remove dead code to improve maintainability.
🔎 Proposed fix
</Select> - {/*<input*/} - {/* type={"text"}*/} - {/* onChange={(e) => setFormat(e.target.value)}*/} - {/* defaultValue={format}*/} - {/*/>*/} </div>
160-160: Apply or remove the commented style attribute.The commented
styleattribute should either be applied or removed for clarity.🔎 Proposed fix
If the style is not needed:
frameBorder="0" - // style="border: none" ></iframe>Or if the style should be applied:
frameBorder="0" - // style="border: none" + style={{ border: 'none' }} ></iframe>components/Clock/Clock.tsx (1)
8-16: Consider setting time immediately on mount to avoid blank flash.The empty initial state ensures hydration consistency, but users will see a blank clock for up to 1 second until the first interval fires. Consider setting the time immediately in the effect before starting the interval:
🔎 Proposed improvement
useEffect(() => { + setTime(moment().locale("en").format(format)) const interval = setInterval(() => { setTime(moment().locale("en").format(format)) }, 1000) return () => clearInterval(interval) }, [format])app/widget/embed/today-prayer-times/page.tsx (1)
3-3: Consider stronger typing forsearchParams.Using
Promise<any>loses type safety. Define an explicit interface for the expected query parameters.🔎 Proposed improvement
-type Props = { searchParams: Promise<any> } +type SearchParams = { + format?: string + sunrise?: string + date?: string + hijri?: string +} + +type Props = { searchParams: Promise<SearchParams> }services/GoogleSheetsService.ts (2)
72-96: Optimize: Run independent data fetches in parallel.Lines 77-80 execute four independent async operations sequentially. Using
Promise.all()would significantly reduce total latency.🔎 Proposed refactor for parallel execution
try { - const configurationData = await sheetsGetConfigurationData() - const prayerTimes = await sheetsGetPrayerData() - const jummahTimes = await sheetsGetJummahData() - const metaData = await sheetsGetMetadata() + const [configurationData, prayerTimes, jummahTimes, metaData] = await Promise.all([ + sheetsGetConfigurationData(), + sheetsGetPrayerData(), + sheetsGetJummahData(), + sheetsGetMetadata(), + ]) return { metadata: metaData,
165-165: Refactor: Remove double type cast.The double cast
as unknown as AnnouncementDataon line 165 indicates a type mismatch. Consider fixing the type definition ofdata.announcementto avoid the need for this cast.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
app/widget/embed/today-prayer-times/TodayPrayerTime.tsxapp/widget/embed/today-prayer-times/page.tsxcomponents/Admin/Embed/EmbedTodayPrayerTimes/EmbedTodayPrayerTimes.tsxcomponents/Clock/Clock.tsxcomponents/Date/Date.tsxcomponents/PrayerTimes/PrayerTimes.tsxnext.config.jspackage.jsonservices/GoogleSheetsService.tstsconfig.json
🧰 Additional context used
🧬 Code graph analysis (4)
app/widget/embed/today-prayer-times/TodayPrayerTime.tsx (4)
types/DailyPrayerTimeType.d.ts (1)
DailyPrayerTime(1-11)services/MosqueDataService.ts (1)
getPrayerTimesForToday(62-66)services/PrayerTimeService.ts (1)
getNextPrayer(30-56)lib/utils.ts (1)
cn(4-6)
services/GoogleSheetsService.ts (6)
types/MosqueDataType.d.ts (2)
MosqueData(5-10)MosqueMetadataType(14-14)config/ConfigurationDefaults.ts (1)
configurationDefaults(3-13)types/DailyPrayerTimeType.d.ts (1)
DailyPrayerTime(1-11)services/GoogleSheetsUtil.ts (4)
prayerTimeValuesToPrayerTimesJsonSchema(115-164)sheetsUtilValuesToJson(15-30)sheetsUtilValuesToNestedJson(32-66)sheetsUtilFlattenedJsonToRows(91-108)types/JummahTimesType.d.ts (1)
JummahTimes(1-1)types/ConfigurationType.d.ts (1)
ConfigurationJson(6-16)
components/PrayerTimes/PrayerTimes.tsx (2)
hooks/useConfiguration.ts (1)
useConfiguration(5-7)services/PrayerTimeService.ts (1)
getNextPrayer(30-56)
app/widget/embed/today-prayer-times/page.tsx (1)
app/widget/embed/today-prayer-times/TodayPrayerTime.tsx (1)
TodayPrayerTime(15-157)
🪛 ast-grep (0.40.3)
app/widget/embed/today-prayer-times/page.tsx
[warning] 28-28: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
components/Admin/Embed/EmbedTodayPrayerTimes/EmbedTodayPrayerTimes.tsx
[error] 17-17: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
app/widget/embed/today-prayer-times/page.tsx
[error] 29-29: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (19)
next.config.js (1)
2-4: No issues found. ThecacheComponents: trueconfiguration is a valid, documented Next.js 16 option that enables the Cache Components model with explicit opt-in caching via the "use cache" directive. The implementation matches the documented syntax correctly.components/Date/Date.tsx (1)
6-7: LGTM! Consistent locale enforcement.Adding explicit
.locale("en")toenglishDateensures English day/month names are displayed regardless of the user's system locale, matching the existinghijriDateformatting.components/PrayerTimes/PrayerTimes.tsx (2)
44-46: Feature flag implementation looks good.The
isTomorrowEnabledflag cleanly gates the tomorrow column rendering, and the hook usage follows the established pattern.
112-127: Tomorrow cell rendering logic is correct.The conditional rendering properly gates on
isTomorrowEnabled, and the highlighting logic correctly checksnextPrayerTime.today === falseto indicate the next prayer is tomorrow.app/widget/embed/today-prayer-times/page.tsx (2)
14-44: Returning raw<html>is unconventional for Next.js App Router.Next.js app router pages typically inherit from a root layout rather than rendering a full HTML document. This works for an embeddable widget but bypasses Next.js's document handling. If this is intentional for iframe isolation, consider adding a comment explaining the design choice.
The
dangerouslySetInnerHTMLflagged by static analysis is a false positive here—the script is a static string literal with no user input, so there's no XSS vector.
34-34:postMessagewith"*"origin is permissive.Using
"*"as the target origin allows any parent window to receive the message. This is typical for embeddable widgets but be aware that any embedding page can listen to these messages.app/widget/embed/today-prayer-times/TodayPrayerTime.tsx (3)
15-28: Server component computesnextPrayerTimeat request time.Since this is an async server component,
getNextPrayer(today)runs once at SSR time. The highlighted "next prayer" will be static and won't update until the page is refreshed. This may be acceptable for an embed widget, but worth noting if real-time highlighting is expected.
8-13: Well-defined props type with constrainedtimeFormat.The union type for
timeFormatprovides clear, valid options and prevents arbitrary format strings.
67-78: Sunrise insertion logic is clean.Inserting Sunrise after Fajr using slice operations is straightforward and maintainable.
tsconfig.json (1)
19-19: Verify the JSX compiler option change in tsconfig.json.The
jsxoption is set to"react-jsx", which deviates from the standard Next.js 16 configuration. Next.js typically uses"preserve"and performs its own optimized JSX transform via SWC. Additionally, Next.js may override thejsxsetting intsconfig.jsonto"preserve"at build time, making this change potentially ineffective.Clarify the rationale for this non-standard configuration and verify that:
- All JSX components build and render correctly with this setting
- This change is not being overridden by Next.js's internal configuration
services/GoogleSheetsService.ts (6)
4-18: LGTM: Imports align with new functionality.The added imports correctly support the new multi-sheet architecture, type-safe configuration merging, and Next.js 15+ caching features.
24-31: Good: Centralized sheet names and client caching.The
SHEET_NAMESconstant improves maintainability. The module-levelsheetsClientsingleton enables connection reuse across requests, which is appropriate for this use case.
34-70: LGTM: Enhanced client initialization with proper caching and connectivity testing.The client caching pattern with early return (line 35) is efficient. The upgrade to
isSheetsClientReady()now verifies actual API connectivity rather than just credential presence, which is more robust.
98-144: LGTM: Well-structured data retrieval functions.These three functions follow a consistent pattern with appropriate data transformations (
prayerTimeValuesToPrayerTimesJsonSchema,sheetsUtilValuesToJson,sheetsUtilValuesToNestedJson) and uniform error handling.
146-161: Excellent: Critical deepmerge issue correctly resolved.The previous critical issue has been properly fixed. The code now correctly converts the 2D array to a nested object using
sheetsUtilValuesToNestedJson(line 155) before merging with defaults, ensuringdeepmergereceives two objects as expected.
182-194: LGTM: Configuration update correctly uses centralized sheet name.The function properly utilizes
SHEET_NAMES.Configurationand correctly transforms the data for the Sheets API.package.json (3)
22-22: LGTM: deepmerge addition.The
deepmergepackage is a well-established utility for deep object merging. Version 4.3.1 is stable and appropriate for configuration merging as described in the PR objectives.
51-51: LGTM: Prettier addition.Adding Prettier as a dev dependency is a good practice for maintaining consistent code formatting across the codebase. Version 3.7.4 is recent and appropriate.
13-13: @radix-ui/react-select@2.2.6 is fully compatible with React 18.2.0.Peer dependencies explicitly support React 18.x (
^18.0), which covers the current version (18.2.0). No compatibility concerns exist.
| function copyUrlToClipboard() { | ||
| window.navigator.clipboard.writeText(widgetUrl) | ||
| setHasCopiedToClipboard(true) | ||
| setTimeout(() => { | ||
| setHasCopiedToClipboard(false) | ||
| }, 3000) | ||
| } |
There was a problem hiding this comment.
Guard against SSR failure when accessing window.navigator.
Direct access to window.navigator.clipboard will throw a ReferenceError during server-side rendering. Add a defensive check before accessing browser APIs.
🔎 Proposed fix with SSR guard
function copyUrlToClipboard() {
+ if (typeof window === 'undefined') return
window.navigator.clipboard.writeText(widgetUrl)
setHasCopiedToClipboard(true)
setTimeout(() => {
setHasCopiedToClipboard(false)
}, 3000)
}📝 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 copyUrlToClipboard() { | |
| window.navigator.clipboard.writeText(widgetUrl) | |
| setHasCopiedToClipboard(true) | |
| setTimeout(() => { | |
| setHasCopiedToClipboard(false) | |
| }, 3000) | |
| } | |
| function copyUrlToClipboard() { | |
| if (typeof window === 'undefined') return | |
| window.navigator.clipboard.writeText(widgetUrl) | |
| setHasCopiedToClipboard(true) | |
| setTimeout(() => { | |
| setHasCopiedToClipboard(false) | |
| }, 3000) | |
| } |
🤖 Prompt for AI Agents
In components/Admin/Embed/EmbedTodayPrayerTimes/EmbedTodayPrayerTimes.tsx around
lines 47 to 53, the function directly accesses window.navigator.clipboard which
will throw during SSR; add a defensive guard (e.g., check typeof window !==
"undefined" and window.navigator?.clipboard) before calling writeText, only call
setHasCopiedToClipboard and the timeout when the clipboard API is available, and
handle the returned Promise rejection (catch) to avoid uncaught errors in
browsers that deny clipboard access; ensure the function is a no-op during
server rendering.
| "clsx": "^2.1.1", | ||
| "deepmerge": "^4.3.1", | ||
| "eslint": "8.48.0", | ||
| "eslint-config-next": "13.4.19", |
There was a problem hiding this comment.
Update eslint-config-next to match Next.js version.
The eslint-config-next package at version 13.4.19 is three major versions behind Next.js 16.1.0. The latest version is 16.1.1, which should match your Next.js version. This version mismatch will likely cause linting configuration issues.
Additionally, Next.js 16 removed the next lint command, so you'll need to update your lint scripts to call ESLint directly and @next/eslint-plugin-next now defaults to ESLint Flat Config format.
🔎 Proposed fix
- "eslint-config-next": "13.4.19",
+ "eslint-config-next": "^16.1.0",Also update your lint script in package.json:
"scripts": {
...
- "lint": "next lint",
+ "lint": "eslint",
},Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In package.json at line 24, eslint-config-next is pinned to 13.4.19 which is
incompatible with Next.js 16.1.0; update the dependency to 16.1.1 to match
Next.js, run install, and ensure package-lock/yarn.lock is updated. Also update
the lint script to call ESLint directly (replace any "next lint" invocation with
an explicit "eslint" command that targets your source files and extensions), and
adjust ESLint config to use the Flat Config format or migrate
@next/eslint-plugin-next settings to the new format as required by Next.js 16.
| "moment-hijri": "^2.1.2", | ||
| "next": "^15.4.1", | ||
| "next": "^16.1.0", | ||
| "next-auth": "^4.24.13", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat package.json | head -50Repository: MosqueOS/Mosque-Prayer-Display-Screen
Length of output: 1463
🏁 Script executed:
npm info next-authRepository: MosqueOS/Mosque-Prayer-Display-Screen
Length of output: 1044
🌐 Web query:
next-auth 4.24.13 Next.js 16 compatibility
💡 Result:
Short answer: No — next-auth 4.24.13 does not declare official Next.js 16 support, so installing it into a Next.js 16 project will produce a peer-dependency conflict unless you override (e.g., --force or --legacy-peer-deps). See the open GitHub issue reporting the peer-dep error and the package release listing (4.24.13). [1][2]
Options:
- Install anyway with npm --legacy-peer-deps or --force (works but bypasses peer-dep checks). [1]
- Track/await an official release that updates next's peerDependency (follow the next-auth repo issue). [1]
- Evaluate next-auth v5/beta for Next.js 16 compatibility (test carefully). [2][5]
Sources:
- next-auth GitHub issue reporting Next.js 16 peer dependency/installation problem. [turn0search0].
- next-auth release listing showing v4.24.13. [turn0search2].
- NextAuth docs / project home. [turn0search5].
Resolve Next.js 16 and next-auth 4.24.13 peer-dependency conflict.
The project uses Next.js 16, but next-auth 4.24.13 does not declare official Next.js 16 support and will produce a peer-dependency conflict during installation. The package can only be installed with --legacy-peer-deps or --force flags, which bypasses dependency checks.
Consider upgrading to next-auth 5.0.0-beta (or later stable when released) for proper Next.js 16 support, or explicitly acknowledge and document the use of legacy peer-dependency overrides if staying on v4.
🤖 Prompt for AI Agents
In package.json around line 31, next-auth@^4.24.13 will produce a peer-dep
conflict with Next.js 16; fix by either upgrading next-auth to a
Next.js‑16‑compatible release (e.g., next-auth 5.0.0‑beta or later) and updating
lockfile/tests, or explicitly document and pin the decision to use legacy peer
dependency resolution (add a README note and CI install flags or use package
manager overrides/resolutions) and run a full install + auth smoke tests to
verify no runtime regressions.
…namically rendered, causing any time or async calls in react components to be surrounded by Suspense (very large change)
…l, supports easier migration and keeping ui comps more dry
… when its after Isha
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
README.md (2)
47-47: Add alt text to all images for accessibility.All images in the screenshots section (lines 47, 49, 53, 57, 61, 62, 66, 70) are still missing alt text attributes, which violates accessibility requirements (WCAG) and markdown lint rules (MD045). This issue was previously flagged but remains unresolved.
Also applies to: 49-49, 53-53, 57-57, 61-61, 62-62, 66-66, 70-70
70-70: Remove trailing slash from image path.The image path has an invalid trailing slash that should be removed.
🔎 Proposed fix
-<img src="./public/screenshots/demo-admin-view.png/"/> +<img src="./public/screenshots/demo-admin-view.png" />components/Admin/Embed/EmbedTodayPrayerTimes/EmbedTodayPrayerTimes.tsx (3)
17-17: Remove the empty object pattern from function parameters.The empty object pattern serves no purpose and is flagged by Biome as a correctness issue.
🔎 Proposed fix
-export default function EmbedTodayPrayerTimes({}) { +export default function EmbedTodayPrayerTimes() {
28-41: Guard against SSR failure when accessingwindow.location.Direct access to
window.location.origincan throw a ReferenceError during server-side rendering. Although this is a client component, defensive coding is recommended.
47-53: Guard against SSR failure when accessingwindow.navigator.Direct access to
window.navigator.clipboardcan throw during SSR and may fail if clipboard permissions are denied. Add defensive checks and error handling.app/widget/embed/today-prayer-times/TodayPrayerTime.tsx (2)
34-35: Past review concern resolved.The previous review flagged missing
.locale("en")onenglishDate. The current implementation usesdtNowFormatFull()anddtNowHijriFormatFull(), which internally calldtNowLocale()and apply the locale. This concern is now addressed.
67-67: Past review concern resolved.The label is now
"Asr"(without leading apostrophe), consistent with other components. This addresses the previous inconsistency concern.services/GoogleSheetsService.ts (1)
157-160: Past critical issue resolved.The deepmerge usage is now correct. The code first converts the 2D array to a nested object using
sheetsUtilValuesToNestedJson, then merges it withconfigurationDefaults. This addresses the previous concern about incompatible types.services/MosqueDataService.ts (2)
35-44: Add error handling for fetch failures.The fetch call lacks response status validation and error handling. Network failures or non-2xx responses will result in uncaught errors or malformed data being returned.
🔎 Proposed fix
} else { - const response = await fetch(MOSQUE_API_ENDPOINT, { - next: { revalidate: 30 }, - }) + try { + const response = await fetch(MOSQUE_API_ENDPOINT, { + next: { revalidate: 30 }, + }) - const data = await response.json() + if (!response.ok) { + throw new Error(`Failed to fetch mosque data: ${response.status} ${response.statusText}`) + } - // we do this so that the MosqueData type doesn't is strongly typed - data.config = unflattenObject(data.config) + const data = await response.json() - return data + // Unflatten the config object to match the MosqueData type structure + data.config = data.config ? unflattenObject(data.config) : configurationDefaults + + return data + } catch (error: any) { + console.error('Failed to fetch mosque data:', error) + throw error + } }
41-42: Fix comment typo and add null safety.The comment has a grammatical error. Additionally,
data.configcould be undefined from the API response.🔎 Proposed fix
- // we do this so that the MosqueData type doesn't is strongly typed - data.config = unflattenObject(data.config) + // Unflatten the config object to match the MosqueData type structure + data.config = data.config ? unflattenObject(data.config) : configurationDefaults
🧹 Nitpick comments (4)
components/Date/Date.tsx (1)
5-5: Consider renaming the component to avoid shadowing the globalDateobject.The component name "Date" conflicts with JavaScript's built-in
Dateconstructor, which can cause confusion. Consider renaming to something more specific likeDateDisplayorMosqueDate.🔎 Proposed fix
-export default function Date() { +export default function DateDisplay() {components/Clock/Clock.tsx (1)
9-21: Consider moving helper function before first usage.The
getCurrentTimeFormattedfunction is defined after it's referenced on Lines 9 and 13. While this works due to function hoisting, it may confuse readers. Consider moving the function definition before Line 9 for better readability.🔎 Suggested placement
export default function Clock({ darkMode = false }: { darkMode?: boolean }) { + function getCurrentTimeFormatted() { + return dtNowLocaleFormatTime12HourWithTimePeriod() + } + const [time, setTime] = useState(getCurrentTimeFormatted()) useEffect(() => { const interval = setInterval(() => { setTime(getCurrentTimeFormatted()) }, 1000) return () => clearInterval(interval) }, []) - - function getCurrentTimeFormatted() { - return dtNowLocaleFormatTime12HourWithTimePeriod() - }components/PrayerTimes/PrayerTimes.tsx (1)
44-46: Add defensive check for config structure.The configuration is accessed directly without checking if the nested properties exist. If
ConfigurationProvideris not properly wrapping this component or if the config structure changes, this could cause a runtime error.🔎 Proposed defensive coding
const config = useConfiguration() const [nextPrayerTime, setNextPrayerTime] = useState(getNextPrayer(today)) -const isTomorrowEnabled = config.feature.prayer_time_tomorrow.enabled +const isTomorrowEnabled = config?.feature?.prayer_time_tomorrow?.enabled ?? falsecomponents/Admin/Announcement/AnnoucementForm.tsx (1)
3-3: React namespace import may be unnecessary.The
Reactnamespace is imported but not directly used in this file. If you're only using hooks (useState,useEffect) and JSX, the namespace import can be removed in modern React (React 17+).🔎 Suggested change
-import React, { useState, useEffect } from 'react' +import { useState, useEffect } from 'react'
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.env.local.exampleREADME.mdapp/widget/embed/today-prayer-times/TodayPrayerTime.tsxapp/widget/page.tsxcomponents/Admin/Announcement/AnnoucementForm.tsxcomponents/Admin/Embed/EmbedTodayPrayerTimes/EmbedTodayPrayerTimes.tsxcomponents/Calendar/Calendar.tsxcomponents/CalendarPrint/PrintStyles/ClassicTableA4/ClassicTableA4.tsxcomponents/CalendarPrint/PrintStyles/SimpleTableA4/SimpleTableA4.tsxcomponents/CalendarPrint/PrintStyles/SimpleTableWhiteA4/SimpleTableWhiteA4.tsxcomponents/Clock/Clock.tsxcomponents/Date/Date.tsxcomponents/PrayerTimes/PrayerTimes.tsxcomponents/SunriseJummahTiles/SunriseJummahTiles.tsxcomponents/UpcomingPrayerDayTiles/UpcomingPrayerDayTiles.tsxlib/datetimeUtils.tsnext.config.jsservices/GoogleSheetsService.tsservices/MosqueDataService.tsservices/PrayerTimeService.ts
✅ Files skipped from review due to trivial changes (1)
- next.config.js
🧰 Additional context used
🧬 Code graph analysis (14)
app/widget/page.tsx (3)
types/DailyPrayerTimeType.d.ts (1)
DailyPrayerTime(1-11)services/MosqueDataService.ts (1)
getPrayerTimesForToday(62-66)lib/datetimeUtils.ts (1)
dtTimeToCustomFormat(41-49)
components/CalendarPrint/PrintStyles/SimpleTableA4/SimpleTableA4.tsx (1)
lib/datetimeUtils.ts (1)
dtLocale(13-15)
components/UpcomingPrayerDayTiles/UpcomingPrayerDayTiles.tsx (1)
lib/datetimeUtils.ts (1)
dtTimeToCustomFormat(41-49)
components/Calendar/Calendar.tsx (1)
lib/datetimeUtils.ts (2)
dtNowLocaleCustomFormat(17-19)dtTimeToCustomFormat(41-49)
components/SunriseJummahTiles/SunriseJummahTiles.tsx (1)
lib/datetimeUtils.ts (1)
dtTimeToCustomFormat(41-49)
services/PrayerTimeService.ts (2)
types/DailyPrayerTimeType.d.ts (1)
DailyPrayerTime(1-11)lib/datetimeUtils.ts (2)
dtNowLocale(9-11)dtLocale(13-15)
components/CalendarPrint/PrintStyles/SimpleTableWhiteA4/SimpleTableWhiteA4.tsx (1)
lib/datetimeUtils.ts (1)
dtLocale(13-15)
components/Admin/Embed/EmbedTodayPrayerTimes/EmbedTodayPrayerTimes.tsx (3)
components/ui/select.tsx (7)
Select(149-149)SelectTrigger(152-152)SelectValue(151-151)SelectContent(153-153)SelectGroup(150-150)SelectLabel(154-154)SelectItem(155-155)components/ui/spinner.tsx (1)
Spinner(17-17)lib/utils.ts (1)
cn(4-6)
components/PrayerTimes/PrayerTimes.tsx (3)
hooks/useConfiguration.ts (1)
useConfiguration(5-7)services/PrayerTimeService.ts (1)
getNextPrayer(30-56)lib/datetimeUtils.ts (1)
dtTimeToCustomFormat(41-49)
components/Clock/Clock.tsx (1)
lib/datetimeUtils.ts (1)
dtNowLocaleFormatTime12HourWithTimePeriod(29-31)
services/MosqueDataService.ts (8)
services/GoogleSheetsUtil.ts (1)
isSheetsClientEnabled(6-8)types/MosqueDataType.d.ts (2)
MosqueData(5-10)MosqueMetadataType(14-14)services/GoogleSheetsService.ts (1)
sheetsGetMosqueData(72-100)lib/unflattenObject.ts (1)
unflattenObject(12-32)lib/datetimeUtils.ts (2)
dtNowLocale(9-11)dtLocale(13-15)types/ConfigurationType.d.ts (1)
ConfigurationJson(6-16)config/ConfigurationDefaults.ts (1)
configurationDefaults(3-13)types/AnnouncementType.d.ts (1)
AnnouncementData(1-10)
components/CalendarPrint/PrintStyles/ClassicTableA4/ClassicTableA4.tsx (1)
lib/datetimeUtils.ts (1)
dtLocale(13-15)
components/Date/Date.tsx (1)
lib/datetimeUtils.ts (1)
dtNowLocaleCustomFormat(17-19)
services/GoogleSheetsService.ts (6)
types/MosqueDataType.d.ts (2)
MosqueData(5-10)MosqueMetadataType(14-14)config/ConfigurationDefaults.ts (1)
configurationDefaults(3-13)services/GoogleSheetsUtil.ts (4)
prayerTimeValuesToPrayerTimesJsonSchema(115-164)sheetsUtilValuesToJson(15-30)sheetsUtilValuesToNestedJson(32-66)sheetsUtilFlattenedJsonToRows(91-108)types/JummahTimesType.d.ts (1)
JummahTimes(1-1)types/ConfigurationType.d.ts (1)
ConfigurationJson(6-16)lib/datetimeUtils.ts (1)
dtNowLocale(9-11)
🪛 Biome (2.1.2)
components/Admin/Embed/EmbedTodayPrayerTimes/EmbedTodayPrayerTimes.tsx
[error] 17-17: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
components/Date/Date.tsx
[error] 5-5: Do not shadow the global "Date" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🪛 dotenv-linter (4.0.0)
.env.local.example
[warning] 9-9: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 9-9: [UnorderedKey] The LOCALE key should go before the THEME_COLOR_HIGHLIGHT key
(UnorderedKey)
🪛 markdownlint-cli2 (0.18.1)
README.md
47-47: Images should have alternate text (alt text)
(MD045, no-alt-text)
49-49: Images should have alternate text (alt text)
(MD045, no-alt-text)
53-53: Images should have alternate text (alt text)
(MD045, no-alt-text)
57-57: Images should have alternate text (alt text)
(MD045, no-alt-text)
61-61: Images should have alternate text (alt text)
(MD045, no-alt-text)
62-62: Images should have alternate text (alt text)
(MD045, no-alt-text)
66-66: Images should have alternate text (alt text)
(MD045, no-alt-text)
70-70: Images should have alternate text (alt text)
(MD045, no-alt-text)
🔇 Additional comments (20)
components/UpcomingPrayerDayTiles/UpcomingPrayerDayTiles.tsx (2)
2-2: LGTM: Clean migration to centralized datetime utility.The import of
dtTimeToCustomFormataligns with the PR's objective to replace Moment.js usage across the app with a centralized datetime utility.
26-58: LGTM: Consistent time formatting across all prayers.The migration from moment-based formatting to
dtTimeToCustomFormatis applied consistently across all five prayer times (Fajr, Zuhr, Asr, Maghrib, Isha) with the same format string"h:mm". The function signature matches the expected usage and handles undefined values gracefully.components/CalendarPrint/PrintStyles/SimpleTableWhiteA4/SimpleTableWhiteA4.tsx (2)
8-8: LGTM: Migration to locale-aware datetime utility.The import of
dtLocalealigns with the PR's objective to centralize datetime handling and add locale support across the application.
93-94: LGTM: Consistent date formatting with locale support.The change from
moment(prayer_time.date)todtLocale(prayer_time.date)maintains the same API while adding locale-awareness. The subsequent.format("ddd")call works identically, asdtLocalereturns a moment object with the configured locale.app/widget/page.tsx (2)
4-6: LGTM: Clean migration to datetime utilities.The import of
dtTimeToCustomFormataligns with the project-wide migration away from moment.js to centralized datetime utilities.
11-13: LGTM: Correct time formatting migration.The migration to
dtTimeToCustomFormatpreserves the original "h:mm" format and maintains the same behavior as the moment-based implementation.components/Date/Date.tsx (1)
3-7: LGTM: Correct date formatting migration.The migration to
dtNowLocaleCustomFormatcorrectly handles both English and Hijri date formatting with appropriate format strings.components/CalendarPrint/PrintStyles/SimpleTableA4/SimpleTableA4.tsx (2)
9-9: LGTM: Correct datetime utility import.The import of
dtLocalealigns with the project-wide migration to centralized datetime utilities.
126-127: LGTM: Correct date parsing migration.The migration to
dtLocalepreserves the existing date parsing and formatting behavior while using the centralized locale-aware utility.services/PrayerTimeService.ts (3)
2-2: LGTM: Correct datetime utility imports.The imports of
dtLocaleanddtNowLocalealign with the project-wide migration to centralized datetime utilities.
6-28: LGTM: Correct migration of time comparison logic.The migration to
dtNowLocale()anddtLocale()in theisBlackoutfunction preserves the existing time comparison logic while using locale-aware utilities.
30-56: LGTM: Correct migration of time comparison logic.The migration to
dtNowLocale()anddtLocale()in thegetNextPrayerfunction preserves the existing time comparison logic while using locale-aware utilities.components/SunriseJummahTiles/SunriseJummahTiles.tsx (2)
2-2: LGTM: Correct datetime utility import.The import of
dtTimeToCustomFormataligns with the project-wide migration to centralized datetime utilities.
20-20: LGTM: Correct time formatting migration.The migration to
dtTimeToCustomFormatfor both sunrise and jummah times preserves the original "h:mm" format and maintains the same behavior as the moment-based implementation.Also applies to: 33-33
components/Calendar/Calendar.tsx (1)
5-116: LGTM! Clean migration to centralized datetime utilities.The replacement of moment-based formatting with
dtTimeToCustomFormatanddtNowLocaleCustomFormatis consistent throughout the component. The new utilities handle edge cases (undefined times) gracefully by returning empty strings, maintaining the same user experience.components/CalendarPrint/PrintStyles/ClassicTableA4/ClassicTableA4.tsx (1)
5-141: LGTM! Consistent migration to dtLocale utility.The replacement of direct moment usage with
dtLocaleis correctly implemented throughout the component. The chaining of.locale("en")for Hijri date calculations is preserved as expected, maintaining the existing date formatting behavior.components/PrayerTimes/PrayerTimes.tsx (2)
7-118: LGTM! Clean migration to dtTimeToCustomFormat.The replacement of moment-based time formatting with
dtTimeToCustomFormatis consistent across all prayer times (start, secondary start, and congregation start). The utility handles edge cases gracefully.
67-121: Feature flag implementation is well-structured.The conditional rendering of the Tomorrow column header and data cells based on
isTomorrowEnabledis correctly implemented. The feature flag cleanly gates the UI without affecting existing functionality.components/Admin/Announcement/AnnoucementForm.tsx (2)
28-43: LGTM! Correct migration to datetime utilities.The replacement of
moment()withdtNowLocale()and the chaineddtLocale().add().format()for end time calculation work correctly. The datetime utilities return moment objects that support the same chaining API.
45-45: Good practice: using empty string instead of null.Using an empty string for
car_reg_numberwhen the type is not "Car" is better than null for form data consistency and prevents potential type issues.
…th number and derive month_label
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
services/GoogleSheetsUtil.ts (1)
143-154: Fix ASR special case handlers to preserve field names.This issue was flagged in a previous review but remains unfixed. The
asr_first_andasr_second_special cases hardcode field names (startandstart_secondary), which discards the actual field suffix. For example, a header likeasr_first_congregation_startwill only setobj.asr.start, losing thecongregation_startpart entirely.Extract and preserve the full field name suffix dynamically, similar to the generic fallback at lines 157-162.
🔎 Proposed fix
// Special case for ASR (first & second) if (header.startsWith('asr_first_')) { obj.asr = obj.asr || {} - obj.asr.start = value + const suffix = header.substring('asr_first_'.length) + obj.asr[suffix || 'start'] = value return } if (header.startsWith('asr_second_')) { obj.asr = obj.asr || {} - obj.asr.start_secondary = value + const suffix = header.substring('asr_second_'.length) + obj.asr[suffix || 'start_secondary'] = value return }
🧹 Nitpick comments (3)
services/GoogleSheetsUtil.ts (2)
7-9: Simplify the double negative logic for clarity.The expression
!(!ADMIN_GOOGLE_SA_EMAIL || !ADMIN_GOOGLE_SA_PRIVATE_KEY)uses a double negative that makes it harder to understand. Consider simplifying:export function isSheetsClientEnabled(): boolean { - return !(!ADMIN_GOOGLE_SA_EMAIL || !ADMIN_GOOGLE_SA_PRIVATE_KEY) + return !!(ADMIN_GOOGLE_SA_EMAIL && ADMIN_GOOGLE_SA_PRIVATE_KEY) }
123-124: Remove or document the//@ts-ignoredirective.The
//@ts-ignoredirective suppresses type checking without explanation. Either remove it if the code can be properly typed, or add a comment explaining why it's necessary.- //@ts-ignore - const obj: any = { } + const obj: Partial<DailyPrayerTime> = { }components/Calendar/Calendar.tsx (1)
125-130: Consider adding an explicit format parameter todtLocalecalls.The current code passes date strings like
"1 January"todtLocalewithout specifying an expected format. While moment's parser may infer the format, explicitly providing"D MMMM"as the second parameter improves reliability and prevents ambiguous parsing in different locales.🔎 Proposed enhancement
- const todayDayName = dtLocale( - `${prayerTime.day_of_month} ${prayerTime.month_label}`, - ).format("ddd") - const todayDayNumAndMonth = dtLocale( - `${prayerTime.day_of_month} ${prayerTime.month_label}`, - ).format("D MMMM") + const todayDayName = dtLocale( + `${prayerTime.day_of_month} ${prayerTime.month_label}`, + "D MMMM" + ).format("ddd") + const todayDayNumAndMonth = dtLocale( + `${prayerTime.day_of_month} ${prayerTime.month_label}`, + "D MMMM" + ).format("D MMMM")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.env.local.examplecomponents/Calendar/Calendar.tsxlib/datetimeUtils.tsservices/GoogleSheetsUtil.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/datetimeUtils.ts
🧰 Additional context used
🧬 Code graph analysis (2)
components/Calendar/Calendar.tsx (1)
lib/datetimeUtils.ts (3)
dtNowLocaleCustomFormat(17-19)dtTimeToCustomFormat(41-49)dtLocale(13-15)
services/GoogleSheetsUtil.ts (2)
types/DailyPrayerTimeType.d.ts (1)
DailyPrayerTime(1-11)lib/datetimeUtils.ts (1)
dtMonthNumToFullMonth(51-55)
🪛 dotenv-linter (4.0.0)
.env.local.example
[warning] 9-9: [UnorderedKey] The LOCALE key should go before the THEME_COLOR_HIGHLIGHT key
(UnorderedKey)
🔇 Additional comments (4)
.env.local.example (1)
8-9: LOCALE configuration looks correct.The quotes issue flagged in the previous review has been addressed. The current value
LOCALE=enis correct for.envfiles.Note: The static analysis hint about key ordering (placing LOCALE before THEME_COLOR_HIGHLIGHT) is cosmetic and entirely optional.
components/Calendar/Calendar.tsx (3)
5-5: LGTM!The import statement correctly brings in the new locale-aware datetime utilities, enabling consistent formatting throughout the component.
86-123: LGTM!The migration from moment-based formatting to
dtTimeToCustomFormatis consistent across all prayer times. The unified approach improves maintainability.
64-70: LGTM!The new Day column header and the updated
colSpan={13}correctly account for the additional column in the table structure.Also applies to: 142-142
| <td | ||
| className={classNames( | ||
| prayerTimeIdx !== prayerTimes.length - 1 | ||
| ? "border-b border-gray-200" | ||
| : "", | ||
| "whitespace-nowrap pl-4 text-left text-sm font-medium", | ||
| )} | ||
| key={`prayerTime_${prayerTime.day_of_month}_${prayerTime.month_label}_day`} | ||
| > | ||
| <a | ||
| href={`#${prayerTime.day_of_month}_${prayerTime.month_label}_day`} | ||
| > | ||
| {todayDayName} | ||
| </a> | ||
| </td> |
There was a problem hiding this comment.
Fix the broken anchor link for the Day column.
The anchor href on line 185 points to #${prayerTime.day_of_month}_${prayerTime.month_label}_day, but the <tr> element on line 158 has an id of ${prayerTime.day_of_month}_${prayerTime.month_label} (without the _day suffix). This mismatch prevents the link from functioning correctly.
🔎 Proposed fix
<a
- href={`#${prayerTime.day_of_month}_${prayerTime.month_label}_day`}
+ href={`#${prayerTime.day_of_month}_${prayerTime.month_label}`}
>
{todayDayName}
</a>📝 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.
| <td | |
| className={classNames( | |
| prayerTimeIdx !== prayerTimes.length - 1 | |
| ? "border-b border-gray-200" | |
| : "", | |
| "whitespace-nowrap pl-4 text-left text-sm font-medium", | |
| )} | |
| key={`prayerTime_${prayerTime.day_of_month}_${prayerTime.month_label}_day`} | |
| > | |
| <a | |
| href={`#${prayerTime.day_of_month}_${prayerTime.month_label}_day`} | |
| > | |
| {todayDayName} | |
| </a> | |
| </td> | |
| <td | |
| className={classNames( | |
| prayerTimeIdx !== prayerTimes.length - 1 | |
| ? "border-b border-gray-200" | |
| : "", | |
| "whitespace-nowrap pl-4 text-left text-sm font-medium", | |
| )} | |
| key={`prayerTime_${prayerTime.day_of_month}_${prayerTime.month_label}_day`} | |
| > | |
| <a | |
| href={`#${prayerTime.day_of_month}_${prayerTime.month_label}`} | |
| > | |
| {todayDayName} | |
| </a> | |
| </td> |
🤖 Prompt for AI Agents
In components/Calendar/Calendar.tsx around lines 175 to 189, the anchor href
includes a trailing "_day" suffix that doesn't match the tr id defined earlier
(which is `${prayerTime.day_of_month}_${prayerTime.month_label}`), breaking the
link; update the anchor href to match the tr id exactly by removing the "_day"
suffix so it becomes `#${prayerTime.day_of_month}_${prayerTime.month_label}`
(alternatively, if you prefer to keep the href as-is, add the same "_day" suffix
to the tr id) so the anchor and target id match.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
package.json (1)
51-51: LGTM! Good addition for code consistency.Adding Prettier as a dev dependency aligns with the PR objectives to ensure consistent formatting. Version 3.7.4 is current and stable.
Consider adding a
.prettierrcorprettier.config.jsfile to define project-specific formatting rules if not already present.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.jsonservices/GoogleSheetsService.ts
🧰 Additional context used
🧬 Code graph analysis (1)
services/GoogleSheetsService.ts (1)
services/GoogleSheetsUtil.ts (4)
prayerTimeValuesToPrayerTimesJsonSchema(116-167)sheetsUtilValuesToJson(16-31)sheetsUtilValuesToNestedJson(33-67)sheetsUtilFlattenedJsonToRows(92-109)
🔇 Additional comments (15)
package.json (4)
16-17: LGTM! Type definitions correctly updated.The React and React DOM type definitions have been properly updated to version 19, matching the runtime React upgrade.
22-22: LGTM! Safe utility addition.The
deepmergelibrary is a stable, well-maintained utility that will work correctly with the configuration system.
30-30: Excellent! React and Next.js successfully upgraded.The upgrade to React 19.2.3 and Next.js 16.1.0 properly addresses the previous compatibility concerns. These versions work together correctly according to the Next.js 16 upgrade guide.
Note: The previously flagged issues with
eslint-config-next(line 24) andnext-auth(line 31) remain unresolved, but those have already been documented in prior review comments.Also applies to: 33-34
13-13: No compatibility issue. @radix-ui/react-select v2.2.6 fully supports React 19 as a peer dependency and will not generate peer dependency warnings.Likely an incorrect or invalid review comment.
services/GoogleSheetsService.ts (11)
1-19: LGTM: Clean imports and proper server directive.The imports are well-organized and the
"use server"directive correctly marks this as a Next.js server action module.
20-29: LGTM: Well-structured constants.Using a
SHEET_NAMESconstant object is a good practice that improves maintainability and reduces the risk of typos.
31-56: LGTM: Efficient client caching with proper validation.The module-level caching of the Sheets client prevents unnecessary re-initialization. Credential validation ensures early failure with clear error messages.
58-69: LGTM: Reliable connectivity check.Testing actual API access rather than just client initialization provides a more robust readiness check.
71-92: LGTM: Well-structured data assembly with appropriate fallbacks.The function properly aggregates data from multiple sources and provides sensible defaults on error, ensuring the application remains functional even when data fetching fails.
94-116: LGTM: Effective caching implementation.The caching pattern with 60-second revalidation strikes a good balance between performance and data freshness for prayer times.
118-140: LGTM: Consistent caching pattern.Follows the same reliable caching pattern established for prayer data.
142-164: LGTM: Proper nested data transformation.Correctly uses
sheetsUtilValuesToNestedJsonfor hierarchical metadata structure.
166-189: LGTM: Deepmerge issue properly resolved.The previous critical issue with
deepmergehas been correctly fixed. The code now properly converts the 2D array to a nested object usingsheetsUtilValuesToNestedJsonbefore merging with defaults, ensuring type compatibility and correct merge behavior.
206-212: LGTM: Clean update flow.The function correctly fetches the current configuration, updates the announcement, and persists the changes.
214-228: LGTM: Correct configuration persistence.The function properly transforms the configuration object to the flattened row format expected by Google Sheets and uses the appropriate
USER_ENTEREDoption.
| export async function sheetsGetAnnouncement(): Promise<AnnouncementData | null> { | ||
| const data = await sheetsGetConfigurationData() | ||
| let announcement = (data?.announcement as unknown as AnnouncementData) ?? null | ||
|
|
||
| const now = dtNowLocale() | ||
| announcement.is_visible = | ||
| now.isSame(announcement?.date, "day") && | ||
| now.isSameOrAfter( | ||
| `${announcement?.date} ${announcement?.start_time}`, | ||
| "minutes", | ||
| ) && | ||
| now.isBefore(`${announcement?.date} ${announcement?.end_time}`, "minutes") | ||
| return announcement | ||
| } |
There was a problem hiding this comment.
Critical null safety violation and cache mutation.
Two issues require immediate attention:
-
Null safety violation (line 196): If
announcementis null (whendata?.announcementis falsy), the code attempts to setannouncement.is_visible, which will throw a runtime error. -
Cache mutation: The function mutates the
is_visibleproperty of the announcement object that's part of the cached configuration data. This can lead to unpredictable behavior across requests.
🔎 Proposed fix
export async function sheetsGetAnnouncement(): Promise<AnnouncementData | null> {
const data = await sheetsGetConfigurationData()
let announcement = (data?.announcement as unknown as AnnouncementData) ?? null
+
+ if (!announcement) {
+ return null
+ }
const now = dtNowLocale()
- announcement.is_visible =
+ // Create a new object to avoid mutating cached data
+ return {
+ ...announcement,
+ is_visible:
- now.isSame(announcement?.date, "day") &&
+ now.isSame(announcement.date, "day") &&
- now.isSameOrAfter(
+ now.isSameOrAfter(
- `${announcement?.date} ${announcement?.start_time}`,
+ `${announcement.date} ${announcement.start_time}`,
- "minutes",
+ "minutes",
- ) &&
+ ) &&
- now.isBefore(`${announcement?.date} ${announcement?.end_time}`, "minutes")
+ now.isBefore(`${announcement.date} ${announcement.end_time}`, "minutes")
+ }
- return announcement
}🤖 Prompt for AI Agents
In services/GoogleSheetsService.ts around lines 191 to 204, the code assigns
announcement.is_visible without checking for null and mutates the cached
configuration object; fix by returning null immediately if announcement is null
(avoid accessing properties on null), then create a shallow clone of the
announcement (so we do not modify the cached object) and compute is_visible on
that clone using safe optional values (use the now and
announcement.date/start_time/end_time only if present), then return the cloned
object.
…rds such as browser international apis, and date time formats
…ars are set rather than checking if sheets client is active as that throws errors
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
components/Admin/Embed/EmbedTodayPrayerTimes/EmbedTodayPrayerTimes.tsx (3)
17-17: Remove the empty object pattern.The empty object pattern
{}in the function signature serves no purpose and is flagged by static analysis. This component doesn't accept props, so the parameter should be omitted entirely.🔎 Proposed fix
-export default function EmbedTodayPrayerTimes({}) { +export default function EmbedTodayPrayerTimes() {
28-41: Guard against SSR failure withwindow.locationaccess.Directly accessing
window.location.originon Line 30 will cause a ReferenceError during server-side rendering sincewindowis undefined on the server. Even with the"use client"directive, defensive coding prevents hydration mismatches and edge-case failures.🔎 Proposed fix with SSR guard
useEffect(() => { setIsIFrameLoading(true) - const url = `${window.location.origin}/widget/embed/today-prayer-times?format=${format}&sunrise=${sunrise}&date=${date}&hijri=${hijri}` + const origin = typeof window !== 'undefined' ? window.location.origin : '' + const url = `${origin}/widget/embed/today-prayer-times?format=${format}&sunrise=${sunrise}&date=${date}&hijri=${hijri}` setWidgetUrl(url) setWidgetIFrameScriptStr(` <iframe src="${url}" width="100%" height="auto" frameborder="0" style="border: none" ></iframe> `) }, [format, sunrise, date, hijri])
47-53: Guard against SSR failure and handle clipboard API errors.Direct access to
window.navigator.clipboardon Line 48 will throw during SSR. Additionally, the clipboard API can fail (e.g., due to permissions), and the unhandled Promise rejection could cause runtime errors.🔎 Proposed fix with SSR guard and error handling
function copyUrlToClipboard() { + if (typeof window === 'undefined' || !window.navigator?.clipboard) return - window.navigator.clipboard.writeText(widgetIFrameScriptStr) + window.navigator.clipboard.writeText(widgetIFrameScriptStr).catch((err) => { + console.error('Failed to copy to clipboard:', err) + }) setHasCopiedToClipboard(true) setTimeout(() => { setHasCopiedToClipboard(false) }, 3000) }services/GoogleSheetsService.ts (1)
178-190: Critical: Null safety violation and cache mutation remain unaddressed.Two critical issues identified in previous reviews are still present:
Null safety violation (Line 185): When
announcementisnull, the code attempts to assignannouncement.is_visible, which will throw a runtime error.Cache mutation (Lines 185-188): The function mutates the
is_visibleproperty of the announcement object that's part of the cached configuration data returned bysheetsGetConfigurationData(). This directly modifies cached state and can cause unpredictable behavior across requests.🔎 Proposed fix to address both issues
export async function sheetsGetAnnouncement(): Promise<AnnouncementData | null> { const data = await sheetsGetConfigurationData() let announcement = (data?.announcement as unknown as AnnouncementData) ?? null + + if (!announcement) { + return null + } const now = dtNowLocale() const announcementDateStart = dtLocale(`${announcement?.date} ${announcement?.start_time}`) const announcementDateEnd = dtLocale(`${announcement?.date} ${announcement?.end_time}`) - announcement.is_visible = + + // Return a new object to avoid mutating cached data + return { + ...announcement, + is_visible: - now.hasSame(announcementDateStart, "day") && + now.hasSame(announcementDateStart, "day") && - now >= announcementDateStart && + now >= announcementDateStart && - now < announcementDateEnd + now < announcementDateEnd + } - return announcement }components/Calendar/Calendar.tsx (1)
175-177: Fix the broken anchor link for the Day column.The anchor
hrefon line 176 points to#${prayerTime.day_of_month}_${prayerTime.month_label}_day, but the<tr>element on line 149 has anidof${prayerTime.day_of_month}_${prayerTime.month_label}(without the_daysuffix). This mismatch prevents the link from functioning correctly.🔎 Proposed fix
<a - href={`#${prayerTime.day_of_month}_${prayerTime.month_label}_day`} + href={`#${prayerTime.day_of_month}_${prayerTime.month_label}`} > {todayDayName} </a>services/MosqueDataService.ts (1)
33-48: Add error handling for API fetch failures.The fetch call lacks response status validation and error handling. If the API returns a non-2xx status or the network request fails, this will result in an uncaught error or malformed data.
🔎 Proposed fix with error handling
export async function getMosqueData (): Promise<MosqueData> { if (useSheetsClient) { return await sheetsGetMosqueData() } else { const response = await fetch(MOSQUE_API_ENDPOINT, { next: { revalidate: 30 }, }) + if (!response.ok) { + throw new Error(`Failed to fetch mosque data: ${response.status} ${response.statusText}`) + } + const data = await response.json() - // we do this so that the MosqueData type doesn't is strongly typed + // Unflatten the config object to match the MosqueData type structure - data.config = unflattenObject(data.config) + data.config = data.config ? unflattenObject(data.config) : {} return data } }
🧹 Nitpick comments (6)
components/Date/Date.tsx (1)
8-8: Consider renaming to avoid shadowing the globalDate.The component function is named
Date, which shadows the globalDateconstructor. While this doesn't cause issues in the current code, it can lead to confusion and potential bugs if developers need to instantiateDateobjects within this file or its children.Consider renaming to something more descriptive like
DateDisplayorCurrentDate:-export default function Date() { +export default function DateDisplay() {components/Calendar/Calendar.tsx (1)
92-95: Remove trailing commas from function calls.Lines 92, 95, and 97 have trailing commas after the argument (e.g.,
dtFormatTo12hAmPm(prayerTime.fajr.start, )). While syntactically valid, this appears unintentional.🔎 Proposed fix
const times = [ - dtFormatTo12hAmPm(prayerTime.fajr.start, ), + dtFormatTo12hAmPm(prayerTime.fajr.start), dtFormatTo12hAmPm(prayerTime.fajr.congregation_start), - dtFormatTo12hAmPm(prayerTime.sunrise_start, ), + dtFormatTo12hAmPm(prayerTime.sunrise_start), - dtFormatTo12hAmPm(prayerTime.zuhr.start, ), + dtFormatTo12hAmPm(prayerTime.zuhr.start),app/widget/embed/today-prayer-times/TodayPrayerTime.tsx (2)
38-49: Clarify the confusingtodayproperty semantics.When
nextPrayerTime.todayisfalse(meaning no more prayers today), it's reset to{ today: true, prayerIndex: 0 }while switching totomorrow's data. Thetodayproperty now means "should highlight a prayer" rather than "is today's date", which is confusing.Consider renaming for clarity:
🔎 Proposed fix
- let nextPrayerTime = getNextPrayer(today) - if (!nextPrayerTime.today) { - nextPrayerTime = { - today: true, - prayerIndex: 0 - } + let nextPrayerTime = getNextPrayer(today) + let shouldHighlight = nextPrayerTime.today + let highlightIndex = nextPrayerTime.prayerIndex + if (!nextPrayerTime.today) { + shouldHighlight = true + highlightIndex = 0 currentDailyPrayerTimes = tomorrowThen update the highlight conditions to use
shouldHighlight && highlightIndex === value.prayerIndex.
32-33: Consider parallelizing independent data fetches.
getPrayerTimesForToday()andgetPrayerTimesForTomorrow()are independent and could be fetched in parallel to reduce latency.🔎 Proposed fix
- const today: DailyPrayerTime = await getPrayerTimesForToday() - const tomorrow: DailyPrayerTime = await getPrayerTimesForTomorrow() + const [today, tomorrow]: [DailyPrayerTime, DailyPrayerTime] = await Promise.all([ + getPrayerTimesForToday(), + getPrayerTimesForTomorrow(), + ])services/MosqueDataService.ts (1)
87-96: Avoid redundant DateTime calculations in loop.
dtNowLocale().plus({ days: index })is computed 4 times per iteration. Cache it once.🔎 Proposed fix
for (let index = 1; index <= days; index++) { + const targetDate = dtNowLocale().plus({ days: index }) let times: UpcomingPrayerTimes = { ...(await getPrayerTimeForDayMonth( - dtFormatDayNumber(dtNowLocale().plus({ days: index })), - dtFormatMonthNumber(dtNowLocale().plus({ days: index })), + dtFormatDayNumber(targetDate), + dtFormatMonthNumber(targetDate), )), - display_date: dtFormatDayDateShort(dtNowLocale() - .plus({ days: index })), - display_day_label: dtFormatDayShort(dtNowLocale().plus({ days: index })), + display_date: dtFormatDayDateShort(targetDate), + display_day_label: dtFormatDayShort(targetDate), }lib/datetimeUtils.ts (1)
162-167: Inconsistent format token:"MMM"vs"LLL"for month short.
dtHijriFormatMonthShortuses"MMM"whiledtFormatMonthShort(line 95) uses"LLL". In Luxon,"LLL"is the standalone short month, while"MMM"is format-context dependent. For consistency, consider using the same token.🔎 Proposed fix
export function dtHijriFormatMonthShort(time?: string | DateTime): string { if (!time) { return "" } - return dtHijri(time).toFormat("MMM") + return dtHijri(time).toFormat("LLL") }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
app/api/data/announcements/route.tsapp/widget/embed/today-prayer-times/TodayPrayerTime.tsxcomponents/Admin/Announcement/AnnoucementForm.tsxcomponents/Admin/Embed/EmbedTodayPrayerTimes/EmbedTodayPrayerTimes.tsxcomponents/Calendar/Calendar.tsxcomponents/CalendarPrint/PrintStyles/ClassicTableA4/ClassicTableA4.tsxcomponents/CalendarPrint/PrintStyles/SimpleTableA4/SimpleTableA4.tsxcomponents/CalendarPrint/PrintStyles/SimpleTableWhiteA4/SimpleTableWhiteA4.tsxcomponents/Clock/Clock.tsxcomponents/Date/Date.tsxlib/datetimeUtils.tspackage.jsonservices/GoogleSheetsService.tsservices/MosqueDataService.tsservices/PrayerTimeService.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- components/CalendarPrint/PrintStyles/SimpleTableA4/SimpleTableA4.tsx
- package.json
- components/Clock/Clock.tsx
- components/CalendarPrint/PrintStyles/SimpleTableWhiteA4/SimpleTableWhiteA4.tsx
- services/PrayerTimeService.ts
- components/Admin/Announcement/AnnoucementForm.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
components/Calendar/Calendar.tsx (1)
lib/datetimeUtils.ts (5)
dtNowLocale(9-11)dtFormatDateMonthLong(112-117)dtFormatTo12hAmPm(56-61)dtLocale(13-24)dtFormatDayShort(70-75)
components/Admin/Embed/EmbedTodayPrayerTimes/EmbedTodayPrayerTimes.tsx (3)
components/ui/select.tsx (7)
Select(149-149)SelectTrigger(152-152)SelectValue(151-151)SelectContent(153-153)SelectGroup(150-150)SelectLabel(154-154)SelectItem(155-155)components/ui/spinner.tsx (1)
Spinner(17-17)lib/utils.ts (1)
cn(4-6)
app/widget/embed/today-prayer-times/TodayPrayerTime.tsx (5)
lib/datetimeUtils.ts (6)
dtTimeToCustomFormat(46-54)dtNowFormatFull(42-44)dtHijriNowFormatFull(144-146)dtFormatDateMonthYearLong(119-124)dtNowLocale(9-11)dtHijriFormatDateMonthYearLong(185-190)types/DailyPrayerTimeType.d.ts (1)
DailyPrayerTime(1-11)services/MosqueDataService.ts (2)
getPrayerTimesForToday(64-71)getPrayerTimesForTomorrow(73-80)services/PrayerTimeService.ts (1)
getNextPrayer(30-57)lib/utils.ts (1)
cn(4-6)
components/Date/Date.tsx (1)
lib/datetimeUtils.ts (2)
dtNowLocaleCustomFormat(26-28)dtHijriNowFormatFull(144-146)
🪛 Biome (2.1.2)
components/Admin/Embed/EmbedTodayPrayerTimes/EmbedTodayPrayerTimes.tsx
[error] 17-17: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
components/Date/Date.tsx
[error] 8-8: Do not shadow the global "Date" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (16)
app/api/data/announcements/route.ts (1)
1-1: LGTM: Cleaner readiness check pattern.The refactor from an asynchronous readiness check to a synchronous
isSheetsClientEnabled()flag simplifies the data source selection logic. The conditional flow correctly routes to the Google Sheets API when enabled, otherwise falling back to the external API.Also applies to: 7-7, 13-17
components/CalendarPrint/PrintStyles/ClassicTableA4/ClassicTableA4.tsx (4)
5-14: LGTM: Comprehensive datetime utility imports.The migration from moment-hijri to the new Luxon-based datetimeUtils introduces locale-aware date formatting. The imported functions cover all necessary operations for Gregorian and Hijri date handling.
56-59: LGTM: Simplified date formatting.The date formatting logic correctly uses the new utilities to derive English and Hijri month names from the first prayer time entry.
103-109: LGTM: Proper date conversion chain.The CalendarRow correctly converts the date string to a locale-aware DateTime, then to Hijri format, and extracts the required components for display.
149-156: LGTM: Correct Hijri range handling.The CalendarHeader properly derives start and end dates using
endOf("month")and formats Hijri years/months. The logic correctly handles month boundary spanning where Hijri months may differ from Gregorian months.services/GoogleSheetsService.ts (6)
24-29: LGTM: Centralized sheet name constants.The
SHEET_NAMESmap provides a single source of truth for sheet identifiers, reducing the risk of typos and simplifying future maintenance.
31-56: LGTM: Proper lazy initialization with credential validation.The
getUserSheetsClientfunction correctly validates service account credentials before initializing the Google Sheets client and caches the client instance for reuse. The error messages are clear and actionable.
58-79: LGTM: Clean data orchestration with fallback defaults.The
sheetsGetMosqueDatafunction properly orchestrates multiple data calls and provides sensible fallback values in case of errors. UsingconfigurationDefaultsas the fallback for config ensures the application remains functional even when the Google Sheets API is unavailable.
81-127: LGTM: Efficient caching strategy for API data.The caching wrappers (
unstable_cache) with 60-second revalidation for prayer and Jummah data provide an excellent balance between freshness and performance, reducing unnecessary API calls to Google Sheets.
153-176: LGTM: Correct deepmerge usage after conversion.The configuration data retrieval correctly converts the 2D array from Google Sheets to a nested object using
sheetsUtilValuesToNestedJsonbefore merging with defaults. This addresses the previous review concern about incompatible types.
200-214: LGTM: Clean configuration update flow.The
sheetsUpdateConfigurationDatafunction correctly converts the configuration JSON to rows and updates the Google Sheets range. The use of theSHEET_NAMESconstant ensures consistency.components/Date/Date.tsx (1)
3-10: LGTM: Clean migration to Luxon-based utilities.The date formatting correctly uses the new
dtNowLocaleCustomFormatanddtHijriNowFormatFullutilities, providing locale-aware Gregorian and Hijri date displays.components/Calendar/Calendar.tsx (1)
4-10: LGTM! Clean migration to Luxon-based datetime utilities.The imports and usage of the new datetime utilities (
dtNowLocale,dtFormatDateMonthLong,dtFormatDayShort,dtFormatTo12hAmPm,dtLocale) are correctly integrated, replacing the previous moment-based implementation.Also applies to: 23-24
app/widget/embed/today-prayer-times/TodayPrayerTime.tsx (1)
102-180: LGTM! Well-structured table rendering with conditional highlighting.The table rendering logic correctly handles optional date/hijri display, prayer slot highlighting based on next prayer, and graceful handling of missing times with empty strings.
services/MosqueDataService.ts (1)
155-159: LGTM! Clean configuration merging with defaults.The
deepmergeusage correctly merges default configuration with fetched config, with proper null coalescing for missing config.lib/datetimeUtils.ts (1)
134-146: LGTM! Hijri calendar integration using Luxon'sislamic-umalquracalendar.The Hijri date functions correctly use
reconfigure({ outputCalendar: "islamic-umalqura" })to output dates in the Islamic calendar system.
| export function dtFormatDayLong(time?: string | DateTime): string { | ||
| if (!time) { | ||
| return "" | ||
| } | ||
| return getDateTimeFromAny(time).toFormat("cccd") | ||
| } |
There was a problem hiding this comment.
Invalid Luxon format token "cccd" in dtFormatDayLong.
The format string "cccd" is non-standard. "ccc" is short weekday (Mon), "cccc" is full weekday (Monday), and "d" is day of month. If the intent is full weekday name, use "cccc".
🔎 Proposed fix
export function dtFormatDayLong(time?: string | DateTime): string {
if (!time) {
return ""
}
- return getDateTimeFromAny(time).toFormat("cccd")
+ return getDateTimeFromAny(time).toFormat("cccc")
}📝 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.
| export function dtFormatDayLong(time?: string | DateTime): string { | |
| if (!time) { | |
| return "" | |
| } | |
| return getDateTimeFromAny(time).toFormat("cccd") | |
| } | |
| export function dtFormatDayLong(time?: string | DateTime): string { | |
| if (!time) { | |
| return "" | |
| } | |
| return getDateTimeFromAny(time).toFormat("cccc") | |
| } |
🤖 Prompt for AI Agents
In lib/datetimeUtils.ts around lines 77 to 82, the Luxon format string "cccd" is
invalid; replace it with the correct token for the intended output. If you want
the full weekday name only, change the format to "cccc"; if you intended full
weekday plus day-of-month, use "cccc d" (or another appropriate token order).
Update the toFormat call accordingly and keep the existing null check and
getDateTimeFromAny usage.
| export function dtHijriFormatYearLong(time?: string | DateTime): string { | ||
| if (!time) { | ||
| return "" | ||
| } | ||
| return dtHijri(time).toFormat("LLLL") | ||
| } |
There was a problem hiding this comment.
Incorrect format token in dtHijriFormatYearLong - returns month instead of year.
The format "LLLL" returns the full month name, not the year. For year output, use "yyyy".
🔎 Proposed fix
export function dtHijriFormatYearLong(time?: string | DateTime): string {
if (!time) {
return ""
}
- return dtHijri(time).toFormat("LLLL")
+ return dtHijri(time).toFormat("yyyy")
}📝 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.
| export function dtHijriFormatYearLong(time?: string | DateTime): string { | |
| if (!time) { | |
| return "" | |
| } | |
| return dtHijri(time).toFormat("LLLL") | |
| } | |
| export function dtHijriFormatYearLong(time?: string | DateTime): string { | |
| if (!time) { | |
| return "" | |
| } | |
| return dtHijri(time).toFormat("yyyy") | |
| } |
🤖 Prompt for AI Agents
In lib/datetimeUtils.ts around lines 178 to 183, dtHijriFormatYearLong uses the
wrong Luxon format token ("LLLL") which returns the full month name; change the
format token to "yyyy" so the function returns the year string (i.e., call
dtHijri(time).toFormat("yyyy") and keep the existing null/empty-time guard).
| const now = dtNowLocale() | ||
| const announcementDateStart = dtLocale( | ||
| `${announcement?.date} ${announcement?.start_time}`, | ||
| ) | ||
|
|
||
| const announcementDateEnd = dtLocale( | ||
| `${announcement?.date} ${announcement?.end_time}`, | ||
| ) |
There was a problem hiding this comment.
Specify format for non-ISO date strings to avoid parsing issues.
dtLocale is called with strings like "${date} ${time}" (e.g., "2024-01-15 09:00"), which uses a space separator. Without a format parameter, dtLocale falls back to DateTime.fromISO(), which expects ISO 8601 format with a T separator. This may produce invalid DateTime objects.
🔎 Proposed fix
- const announcementDateStart = dtLocale(
- `${announcement?.date} ${announcement?.start_time}`,
- )
- const announcementDateEnd = dtLocale(
- `${announcement?.date} ${announcement?.end_time}`,
- )
+ const announcementDateStart = dtLocale(
+ `${announcement?.date} ${announcement?.start_time}`,
+ "yyyy-MM-dd HH:mm",
+ )
+ const announcementDateEnd = dtLocale(
+ `${announcement?.date} ${announcement?.end_time}`,
+ "yyyy-MM-dd HH:mm",
+ )Alternatively, use ISO format with T separator:
- const announcementDateStart = dtLocale(
- `${announcement?.date} ${announcement?.start_time}`,
- )
+ const announcementDateStart = dtLocale(
+ `${announcement?.date}T${announcement?.start_time}`,
+ )📝 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 now = dtNowLocale() | |
| const announcementDateStart = dtLocale( | |
| `${announcement?.date} ${announcement?.start_time}`, | |
| ) | |
| const announcementDateEnd = dtLocale( | |
| `${announcement?.date} ${announcement?.end_time}`, | |
| ) | |
| const now = dtNowLocale() | |
| const announcementDateStart = dtLocale( | |
| `${announcement?.date} ${announcement?.start_time}`, | |
| "yyyy-MM-dd HH:mm", | |
| ) | |
| const announcementDateEnd = dtLocale( | |
| `${announcement?.date} ${announcement?.end_time}`, | |
| "yyyy-MM-dd HH:mm", | |
| ) |
🤖 Prompt for AI Agents
In services/MosqueDataService.ts around lines 169 to 175, dtLocale is being
called with strings like "${date} ${time}" which are not ISO and will be passed
to DateTime.fromISO(), causing invalid DateTime objects; fix by either (a)
passing an explicit format to dtLocale (e.g., parse with format "yyyy-LL-dd
HH:mm" or the project’s expected format) so the parser uses DateTime.fromFormat,
or (b) build an ISO string with a 'T' separator (e.g., `${date}T${time}`) before
calling dtLocale; update both announcementDateStart and announcementDateEnd to
use one of these approaches consistently.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
lib/datetimeUtils.ts (2)
185-190: Critical:dtHijriFormatYearLongreturns month name instead of year.The format token
"LLLL"produces the full month name (e.g., "Ramadan"), not the year. For year output, use"yyyy".This issue was flagged in a previous review but remains unresolved.
🔎 Proposed fix
export function dtHijriFormatYearLong(time?: string | DateTime): string { if (!time) { return "" } - return dtHijri(time).toFormat("LLLL") + return dtHijri(time).toFormat("yyyy") }
84-89: Invalid Luxon format token"cccd"- produces incorrect output.The format string
"cccd"is non-standard. In Luxon:
"cccc"= full weekday name (Monday, Tuesday)"d"= day of month (1-31)Concatenating them as
"cccd"produces malformed output. Use"cccc"for full weekday only, or"cccc d"if you need both weekday and day number.This issue was flagged in a previous review but remains unresolved.
🔎 Proposed fix
export function dtFormatDayLong(time?: string | DateTime): string { if (!time) { return "" } - return dtLocale(time).toFormat("cccd") + return dtLocale(time).toFormat("cccc") }Or if you need both weekday and day number:
- return dtLocale(time).toFormat("cccd") + return dtLocale(time).toFormat("cccc d")
🧹 Nitpick comments (1)
lib/datetimeUtils.ts (1)
12-14:dtNowLocale()is identical todtNow()and serves no distinct purpose.Since
Settings.defaultLocaleis already set globally, both functions return the same result. Consider either removingdtNowLocale()or making its purpose explicit by calling.setLocale(LOCALE)for clarity.This was flagged in a previous review but remains unresolved.
🔎 Proposed fix
export function dtNowLocale(): DateTime { - return DateTime.now() + return DateTime.now().setLocale(LOCALE) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/datetimeUtils.ts
🔇 Additional comments (3)
lib/datetimeUtils.ts (3)
1-6: LGTM - Locale configuration is correct.Setting
Settings.defaultLocaleensures all DateTime instances inherit the configured locale by default.
16-31: LGTM - Input sanitization and parsing logic are sound.The
.trim()call at line 26 properly handles whitespace-related parsing issues as noted in the commit message.
33-68: LGTM - Formatting helpers are implemented correctly.The functions follow a consistent pattern with proper null checks and valid Luxon format tokens.
|
Changes migrated to new PR: #45 |
1 participant
feat: add ConfigurationType.d.ts and defaults so we can strongly type the google sheet config data, add option to enable/disable tomorrow salah times
Summary by CodeRabbit
New Features
UI
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.