feature: Admin Page and Announcements #39
Conversation
…cements. - added admin page with username/password authentication which is set by ENV vars - integrated google sheets api, requiring google credentials to be used to update the sheets (client side oAuth was reverted as it required google cloud projects to be verified)
…r relevant message
…ble or not, we'll use the username/password and the google service account creds. - add some error handling to the announcement route so we can show the error in the UI - formatting changes
|
@mahfuzhannan is attempting to deploy a commit to the MosqueOS Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughAdds an admin interface with NextAuth (credentials/Google), announcement creation/display UIs, Google Sheets-backed storage and API routes, supporting utility functions, UI primitives, new types, environment variables, test/jest configuration, and related docs/config updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant NextServer
participant SessionStore
participant MosqueDataService
participant GoogleSheets
User->>Browser: Navigate to /admin
Browser->>NextServer: GET /admin
NextServer->>SessionStore: getSession(authOptions)
alt No session
NextServer->>Browser: Redirect to /api/auth/signin
else Session exists
NextServer->>MosqueDataService: getMetaData()
MosqueDataService->>GoogleSheets: sheetsReadConfiguration()
GoogleSheets-->>MosqueDataService: configuration
NextServer->>Browser: Render AdminPage (SessionProviderWrapper)
end
sequenceDiagram
actor Admin
participant Browser
participant AddAnnouncement
participant Modal
participant AnnouncementForm
participant MosqueDataService
participant GoogleSheets
participant APIRoute
Admin->>Browser: Click "Add Announcement"
Browser->>AddAnnouncement: open modal
AddAnnouncement->>Modal: render AnnouncementForm
Admin->>AnnouncementForm: submit(formData)
AnnouncementForm->>MosqueDataService: createAnnouncement(formData)
MosqueDataService->>GoogleSheets: sheetsUpdateConfiguration()
GoogleSheets-->>MosqueDataService: success
MosqueDataService-->>AnnouncementForm: resolved
AnnouncementForm->>AddAnnouncement: onComplete(updated)
AddAnnouncement->>Modal: close()
APIRoute->>MosqueDataService: GET returns updated announcement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 13
🧹 Nitpick comments (14)
types/AnnouncementType.d.ts (1)
1-10: Consider documenting whyduration_minutesis a string.The field name suggests a numeric value, but it's typed as
string | null. If this is intentional (e.g., for Google Sheets compatibility), consider adding a comment to clarify the design decision..env.local.example (2)
14-16: Provide guidance for generating a secureAUTH_SECRET.The example value
"some-long-random-string"is insufficient. NextAuth requires a strong, cryptographically random secret for security. Add a comment explaining how to generate a secure value.Apply this diff to add guidance:
+# Generate a secure secret with: openssl rand -base64 32 AUTH_USERNAME=myuser AUTH_PASSWORD=secret AUTH_SECRET=some-long-random-string
11-11: Consider addressing static analysis suggestions (optional).Static analysis flagged minor style issues:
- Key ordering: place
ADMIN_GOOGLE_SA_EMAILbeforeADMIN_GOOGLE_SA_PRIVATE_KEY, andAUTH_PASSWORDandAUTH_SECRETbeforeAUTH_USERNAME- Add a blank line at the end of the file
These are optional improvements for consistency.
Also applies to: 15-16
README.md (1)
146-146: Add language identifiers to code blocks.The fenced code blocks on lines 146 and 155 should specify a language for better rendering and syntax highlighting.
Add
bashorenvlanguage identifiers:-``` +```bash e.g.-``` +```env AUTH_USERNAME=myuser AUTH_PASSWORD=secretAlso applies to: 155-155 </blockquote></details> <details> <summary>app/api/data/announcements/route.ts (3)</summary><blockquote> `7-7`: **Unused parameter.** The `request: NextRequest` parameter is declared but never used in the handler. If you don't need to read query parameters or headers, remove it: ```diff -export async function GET(request: NextRequest,) { +export async function GET() {Or if cache busting is important, you could verify the no_cache param is present for debugging purposes (though this is optional).
7-7: Remove trailing comma.There's an unnecessary trailing comma after the
NextRequestparameter.-export async function GET(request: NextRequest,) { +export async function GET(request: NextRequest) {
11-12: Consider more specific error status codes.Returning 400 (Bad Request) for all errors is misleading. A 400 status indicates a client error, but failures in
getAnnouncement()(like Google Sheets API errors) are server errors and should return 500.} catch (error: any) { - return Response.json({ error: error?.message ?? "Unknown error" }, { status: 400 }); + return Response.json({ error: error?.message ?? "Unknown error" }, { status: 500 }); }app/admin/SessionProviderWrapper.tsx (1)
6-8: Complete the documentation comment.The comment is incomplete: "So that we don't have to wrap the whole app in a session provider and keep" - the sentence trails off.
Complete the thought or simplify:
/** - * So that we don't have to wrap the whole app in a session provider and keep - * @param children - * @constructor + * Wraps children with NextAuth SessionProvider for admin pages only. + * This avoids wrapping the entire app when session is only needed in /admin. */components/Announcement/Announcement.tsx (1)
11-24: Optionally trigger an immediate fetch before starting the polling intervalRight now the first announcement won’t be fetched until 10 seconds after mount. If you want announcements to appear as soon as the page loads, consider doing one
fetchcall beforesetInterval(or calling the polling function once before registering the interval).app/admin/page.tsx (1)
9-60: Admin gating and metadata usage look correct; consider dropping unnecessaryasyncThe feature-flag + session check flow in
AdminPageWrapperand the metadata generation both look good for the admin route.You can optionally remove
asyncfromAdminServerPagesince it doesn’tawaitanything; that will slightly simplify typing and make intent clearer.components/Admin/Announcement/AnnoucementForm.tsx (1)
25-36: Align default start time with the “+2 minutes” commentThe comment says “Start time = now + 2 minutes”, but
startis just a clone ofnowwith no offset. If you really want a 2‑minute offset, adjust the timestamp:- // Start time = now + 2 minutes - const start = new Date(now.getTime()) + // Start time = now + 2 minutes + const start = new Date(now.getTime() + 2 * 60 * 1000)services/GoogleSheetsService.ts (3)
8-37: Validate required Google Sheets env vars up front for clearer failuresRight now
SPREADSHEET_IDcan be''and the service account email/key can beundefined, which will only surface later as opaque Google API/auth errors. It’s better to fail fast with a clear message when configuration is missing.You can keep the call sites unchanged and add a simple guard:
-const SPREADSHEET_ID = process.env.SPREADSHEET_ID ?? '' -const ADMIN_GOOGLE_SA_PRIVATE_KEY = process.env.ADMIN_GOOGLE_SA_PRIVATE_KEY -const ADMIN_GOOGLE_SA_EMAIL = process.env.ADMIN_GOOGLE_SA_EMAIL +const SPREADSHEET_ID = process.env.SPREADSHEET_ID +const ADMIN_GOOGLE_SA_PRIVATE_KEY = process.env.ADMIN_GOOGLE_SA_PRIVATE_KEY +const ADMIN_GOOGLE_SA_EMAIL = process.env.ADMIN_GOOGLE_SA_EMAIL @@ export async function getUserSheetsClient () { const session = await getSession() // next-auth v5 app router helper if (!session) { throw new Error('Not authenticated with Google') } + if (!SPREADSHEET_ID || !ADMIN_GOOGLE_SA_EMAIL || !ADMIN_GOOGLE_SA_PRIVATE_KEY) { + throw new Error('Google Sheets service account env vars are not fully configured') + } + try { @@ - const googleAuthJwt = new google.auth.JWT({ - email: ADMIN_GOOGLE_SA_EMAIL, - key: ADMIN_GOOGLE_SA_PRIVATE_KEY?.replace(/\\n/g, '\n'), + const googleAuthJwt = new google.auth.JWT({ + email: ADMIN_GOOGLE_SA_EMAIL, + key: ADMIN_GOOGLE_SA_PRIVATE_KEY.replace(/\\n/g, '\n'), scopes: ['https://www.googleapis.com/auth/spreadsheets'], })
39-51: Harden announcement JSON parsing against corrupt sheet data
sheetsGetAnnouncementassumes the metadata cell always contains valid JSON. A malformed value will causeJSON.parseto throw and break the admin flow.Consider making parsing defensive so a bad cell just results in “no announcement” (or a logged warning) instead of a crash:
export async function sheetsGetAnnouncement (): Promise<AnnouncementData | null> { const data = await sheetsGetMetadataValueByKey('announcement') - if (data) { - return JSON.parse(data) as AnnouncementData - } - return null + if (!data) return null + + try { + return JSON.parse(data) as AnnouncementData + } catch (error) { + console.error('Failed to parse announcement JSON from Metadata sheet', error) + return null + } }
85-121: Metadata update helper error message should include the actual key
sheetsUpdateMetadataValueByKeythrows if the key row is missing, but the error message always says it couldn’t find'announcement', which is misleading when this helper is used for other metadata keys.You can keep the behavior (throw on missing key) while making the message accurate:
const targetIndex = rows.findIndex( (row) => row[0]?.trim() === key, ) if (targetIndex === -1) { - throw new Error( - `Could not find 'announcement' row in sheet ${SHEET_NAME_METADATA}`) + throw new Error( + `Could not find '${key}' row in sheet ${SHEET_NAME_METADATA}`) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
.env.local.example(1 hunks).gitignore(1 hunks)README.md(8 hunks)app/admin/SessionProviderWrapper.tsx(1 hunks)app/admin/page.tsx(1 hunks)app/api/auth/[...nextauth]/route.ts(1 hunks)app/api/data/announcements/route.ts(1 hunks)app/auth.ts(1 hunks)app/page.tsx(3 hunks)components/Admin/AdminPage.tsx(1 hunks)components/Admin/Announcement/AddAnnouncement.tsx(1 hunks)components/Admin/Announcement/AnnoucementForm.tsx(1 hunks)components/Announcement/AnnoucementModal.tsx(1 hunks)components/Announcement/Announcement.tsx(1 hunks)components/ui/Buttons.tsx(1 hunks)components/ui/LoadingSpinner.tsx(1 hunks)components/ui/Modal.tsx(1 hunks)package.json(1 hunks)public/service-worker.js(1 hunks)services/GoogleSheetsService.ts(1 hunks)services/MosqueDataService.ts(2 hunks)tsconfig.json(2 hunks)types/AnnouncementType.d.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
app/api/data/announcements/route.ts (1)
services/MosqueDataService.ts (1)
getAnnouncement(92-108)
app/admin/page.tsx (6)
app/page.tsx (1)
generateMetadata(28-35)types/MosqueDataType.d.ts (1)
MosqueMetadataType(10-10)services/MosqueDataService.ts (1)
getMetaData(86-90)app/admin/SessionProviderWrapper.tsx (1)
SessionProviderWrapper(10-12)app/auth.ts (3)
isAdminInterfaceEnabled(80-80)getSession(80-80)redirect(54-57)components/Admin/AdminPage.tsx (1)
AdminPage(5-33)
components/Announcement/Announcement.tsx (2)
types/AnnouncementType.d.ts (1)
AnnouncementData(1-10)components/Announcement/AnnoucementModal.tsx (1)
AnnouncementModal(8-30)
components/Admin/Announcement/AddAnnouncement.tsx (5)
types/AnnouncementType.d.ts (1)
AnnouncementData(1-10)components/ui/LoadingSpinner.tsx (1)
LoadingSpinner(3-11)components/ui/Buttons.tsx (1)
PrimaryButton(4-24)components/ui/Modal.tsx (1)
Modal(21-91)components/Admin/Announcement/AnnoucementForm.tsx (1)
AnnouncementForm(13-168)
components/Admin/Announcement/AnnoucementForm.tsx (3)
types/AnnouncementType.d.ts (1)
AnnouncementData(1-10)services/MosqueDataService.ts (1)
createAnnouncement(110-112)components/ui/LoadingSpinner.tsx (1)
LoadingSpinner(3-11)
components/Admin/AdminPage.tsx (2)
types/MosqueDataType.d.ts (1)
MosqueMetadataType(10-10)components/Admin/Announcement/AddAnnouncement.tsx (1)
AddAnnouncement(10-81)
services/MosqueDataService.ts (2)
types/AnnouncementType.d.ts (1)
AnnouncementData(1-10)services/GoogleSheetsService.ts (2)
sheetsGetAnnouncement(39-46)sheetsUpdateAnnouncement(48-51)
services/GoogleSheetsService.ts (2)
types/AnnouncementType.d.ts (1)
AnnouncementData(1-10)types/DailyPrayerTimeType.d.ts (1)
DailyPrayerTime(1-11)
app/api/auth/[...nextauth]/route.ts (1)
app/auth.ts (1)
authOptions(80-80)
app/page.tsx (2)
components/UpcomingPrayerDayTiles/UpcomingPrayerDayTiles.tsx (1)
UpcomingPrayerDayTiles(4-63)components/Announcement/Announcement.tsx (1)
Announcement(7-70)
🪛 Biome (2.1.2)
components/Admin/Announcement/AddAnnouncement.tsx
[error] 10-11: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
🪛 dotenv-linter (4.0.0)
.env.local.example
[warning] 11-11: [UnorderedKey] The ADMIN_GOOGLE_SA_EMAIL key should go before the ADMIN_GOOGLE_SA_PRIVATE_KEY key
(UnorderedKey)
[warning] 15-15: [UnorderedKey] The AUTH_PASSWORD key should go before the AUTH_USERNAME key
(UnorderedKey)
[warning] 16-16: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
[warning] 16-16: [UnorderedKey] The AUTH_SECRET key should go before the AUTH_USERNAME key
(UnorderedKey)
🪛 LanguageTool
README.md
[style] ~99-~99: Consider using a different verb to strengthen your wording.
Context: ...n generate your own endpoint but please make sure it has all the required properties. ##...
(MAKE_SURE_ENSURE)
[uncategorized] ~189-~189: “Google” is a proper noun and needs to be capitalized.
Context: ...f Admin interface to interact with your google sheets | | ADMIN_GOOGLE_SA_EMAI...
(A_GOOGLE)
[uncategorized] ~190-~190: “Google” is a proper noun and needs to be capitalized.
Context: ...f Admin interface to interact with your google sheets | | AUTH_USERNAME ...
(A_GOOGLE)
🪛 markdownlint-cli2 (0.18.1)
README.md
22-22: Link text should be descriptive
(MD059, descriptive-link-text)
129-129: Bare URL used
(MD034, no-bare-urls)
134-134: Bare URL used
(MD034, no-bare-urls)
146-146: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
155-155: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
179-179: Bare URL used
(MD034, no-bare-urls)
190-190: Bare URL used
(MD034, no-bare-urls)
214-214: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (19)
public/service-worker.js (1)
30-33: LGTM! This follows service worker best practices.Restricting the service worker to only handle GET requests is correct. Non-GET requests (POST, PUT, DELETE) should not be cached since they are not idempotent and may have side effects. This is particularly important for the admin functionality being added in this PR, where authentication and announcement creation will use POST requests.
.gitignore (1)
21-21: LGTM!Adding
.ideato.gitignoreis appropriate for excluding JetBrains IDE configuration files from version control.components/ui/LoadingSpinner.tsx (1)
1-11: LGTM!The LoadingSpinner component is clean and straightforward. The implementation correctly uses Tailwind CSS for styling and animations.
tsconfig.json (1)
31-40: LGTM!The TypeScript configuration updates are appropriate. Adding
.next/dev/types/**/*.tsto the include patterns aligns with Next.js 15 development type generation, and the formatting changes improve readability.package.json (1)
19-19: No active security vulnerabilities detected for the added versions.Verification shows
googleapis@^105.0.0andnext-auth@^4.24.13have no known security advisories:
- googleapis 105.0.0 is not affected by the documented HIGH severity authorization vulnerability (affects only < 39.1.0)
- next-auth 4.24.13 is not affected by any documented advisories, including the recent MODERATE email misdelivery vulnerability (affects < 4.24.12) and the HIGH OAuth vulnerability (affects < 4.20.1)
While newer versions are available, the specified versions are stable and secure for use.
README.md (1)
60-60: LGTM! Helpful clarification.Good addition to clarify that Google Cloud Project is optional and only needed for the admin interface.
components/ui/Modal.tsx (3)
6-19: LGTM! Well-typed modal props.The type definitions are clear and complete, with sensible defaults.
30-39: LGTM! Proper keyboard event handling.The ESC key handler is correctly implemented with proper cleanup.
49-49: Backdrop click is disabled.The backdrop
onClick={onClose}is commented out, meaning users cannot close the modal by clicking outside it. This is a valid design choice but differs from common modal UX patterns.Confirm this is intentional. If users should be able to close by clicking the backdrop, uncomment line 49.
app/page.tsx (2)
53-55: LGTM! Proper React key added.Adding
key={times.display_date}correctly satisfies React's requirement for list items and uses an appropriate unique identifier.
84-84: LGTM! Announcement component integrated.The component is correctly placed at the page level and will manage its own state and fetching logic.
components/Admin/AdminPage.tsx (2)
1-9: LGTM! Component structure is clear.The component signature and imports are well-organized.
11-33: LGTM! Clean admin page layout.The page structure is well-organized with a branded header and content area. The responsive design uses appropriate Tailwind breakpoints.
app/api/auth/[...nextauth]/route.ts (1)
1-6: LGTM! Standard NextAuth route setup.This follows the recommended pattern for NextAuth in Next.js App Router, correctly exporting both GET and POST handlers.
app/auth.ts (3)
14-28: Google OAuth configured with forced consent.The configuration is correct, but note that
prompt: "consent"will show the consent screen on every login, which may impact UX.If this is intentional (to ensure you always get a refresh token), this is fine. Otherwise, consider
prompt: "select_account"which only prompts when necessary:params: { access_type: "offline", - prompt: "consent", + prompt: "select_account", scope: "openid email profile https://www.googleapis.com/auth/spreadsheets"
51-63: LGTM! Auth configuration is appropriate.The redirect behavior and theme configuration are correctly implemented.
65-79: LGTM! Helpful utility functions.Both helper functions are well-implemented. The
isAdminInterfaceEnabledlogic correctly checks for either authentication method.components/Admin/Announcement/AddAnnouncement.tsx (1)
36-77: Announcement card + modal wiring looks coherentThe state flow from initial fetch →
currentAnnouncement→ details view, plus opening the modal and updatingcurrentAnnouncementviaonComplete, is consistent and easy to follow.services/MosqueDataService.ts (1)
92-112: Announcement visibility calculation looks good; be aware of server timezoneThe
getAnnouncementhelper cleanly centralizes theis_visiblecomputation based ondate,start_time, andend_time, and returnsnullwhen no date is set, which matches how the UI handles “no announcement”.Just note that
moment()here uses the server’s local timezone; if you ever deploy across different timezones or move to UTC‑based data, you may want to standardize this to avoid surprises.
| import React from 'react' | ||
|
|
||
| type AnnouncementModalProps = { | ||
| isOpen: boolean; | ||
| children: React.ReactNode; | ||
| }; | ||
|
|
||
| export default function AnnouncementModal({ | ||
| isOpen, | ||
| children, | ||
| }: AnnouncementModalProps) { | ||
| if (!isOpen) return null; | ||
|
|
||
| return ( | ||
| <div | ||
| className="fixed inset-0 z-[9999] flex items-center justify-center bg-black/50 backdrop-blur-sm" | ||
| aria-modal="true" | ||
| role="dialog" | ||
| > | ||
| <div | ||
| className={`w-full max-w-6xl mx-4 rounded-2xl bg-white shadow-xl`} | ||
| onClick={(e) => e.stopPropagation()} // prevent close when clicking inside | ||
| > | ||
| <div className="px-2 py-4 text-sm text-slate-700"> | ||
| {children} | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Filename typo: "Annoucement" should be "Announcement".
The filename is AnnoucementModal.tsx (missing an 'n'), while the component is correctly named AnnouncementModal. This inconsistency can cause confusion and import issues.
Rename the file to AnnouncementModal.tsx and verify all imports are updated:
🏁 Script executed:
#!/bin/bash
# Search for imports of the misspelled filename
rg -n "AnnoucementModal" --type ts --type tsxLength of output: 27
🏁 Script executed:
#!/bin/bash
# Search for imports of the misspelled filename - corrected syntax
rg -n "AnnoucementModal" --type-list | grep -E "(ts|tsx)" || rg -n "AnnoucementModal" -g "*.ts" -g "*.tsx"Length of output: 511
🏁 Script executed:
#!/bin/bash
# Search for all references to the misspelled filename
rg -n "AnnoucementModal" -g "*.ts" -g "*.tsx"Length of output: 121
Rename AnnoucementModal.tsx to AnnouncementModal.tsx and update the import in git/components/Announcement/Announcement.tsx line 5.
The filename contains a typo (missing 'n') while the component is correctly named AnnouncementModal. The misspelled filename is currently imported in git/components/Announcement/Announcement.tsx.
🤖 Prompt for AI Agents
In components/Announcement/AnnoucementModal.tsx around lines 1 to 30, the
filename is misspelled (AnnoucementModal.tsx) while the component is correctly
named AnnouncementModal; rename the file to
components/Announcement/AnnouncementModal.tsx and update the import statement on
git/components/Announcement/Announcement.tsx at line 5 to import from
"./AnnouncementModal" (adjust path if needed), then run a quick project-wide
search to update any other imports and commit the rename so exports and import
paths remain consistent.
services/GoogleSheetsService.ts
Outdated
| async function sheetsGetMetadataValueByKey (key: string): Promise<string | null> { | ||
| try { | ||
| const sheets = await getUserSheetsClient() | ||
| const metadataValueRange = await sheets.spreadsheets.values.get({ | ||
| spreadsheetId: SPREADSHEET_ID, | ||
| range: SHEET_NAME_METADATA, // or your sheet name | ||
| }) | ||
| const rows = metadataValueRange.data.values || [] | ||
| const targetIndex = rows.findIndex( | ||
| (row) => row[0]?.trim() === key, | ||
| ) | ||
|
|
||
| if (targetIndex === -1) { | ||
| throw new Error( | ||
| `Could not find 'announcement' row in sheet ${SHEET_NAME_METADATA}`) | ||
| } | ||
|
|
||
| // The value is in column B = column index 1 | ||
| const cell = `${SHEET_NAME_METADATA}!B${targetIndex + 1}` // add 1 because Google Sheets is 1-indexed | ||
|
|
||
| const data = await sheets.spreadsheets.values.get({ | ||
| spreadsheetId: SPREADSHEET_ID, | ||
| range: cell, | ||
| }) | ||
|
|
||
| return data?.data?.values?.[0]?.[0] ?? null | ||
| } catch (error: any) { | ||
| console.error(error) | ||
| throw new Error(`Google Sheets API request failed: ${error?.message}`) | ||
| } | ||
| } |
There was a problem hiding this comment.
Behavior of sheetsGetMetadataValueByKey doesn’t match its signature and hardcodes 'announcement'
Two issues here:
- The function is typed to return
Promise<string | null>, but a missing key causes it to throw instead of returningnull. That meanssheetsGetAnnouncementcan never see a “no data” state; it only gets a value or an exception. - The error message hardcodes
'announcement', even thoughkeycan be anything.
If you want “null if key missing” semantics (which aligns with the return type and announcement use case), you can change the missing-row branch to return null and fix the message:
const rows = metadataValueRange.data.values || []
const targetIndex = rows.findIndex(
(row) => row[0]?.trim() === key,
)
if (targetIndex === -1) {
- throw new Error(
- `Could not find 'announcement' row in sheet ${SHEET_NAME_METADATA}`)
+ // Key not present in Metadata sheet
+ return null
}
@@
} catch (error: any) {
console.error(error)
- throw new Error(`Google Sheets API request failed: ${error?.message}`)
+ throw new Error(`Google Sheets API request failed for key '${key}': ${error?.message}`)
}If you do intend absence to be exceptional, I’d recommend updating the return type to Promise<string> and updating the error message to interpolate key instead of 'announcement'.
📝 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.
| async function sheetsGetMetadataValueByKey (key: string): Promise<string | null> { | |
| try { | |
| const sheets = await getUserSheetsClient() | |
| const metadataValueRange = await sheets.spreadsheets.values.get({ | |
| spreadsheetId: SPREADSHEET_ID, | |
| range: SHEET_NAME_METADATA, // or your sheet name | |
| }) | |
| const rows = metadataValueRange.data.values || [] | |
| const targetIndex = rows.findIndex( | |
| (row) => row[0]?.trim() === key, | |
| ) | |
| if (targetIndex === -1) { | |
| throw new Error( | |
| `Could not find 'announcement' row in sheet ${SHEET_NAME_METADATA}`) | |
| } | |
| // The value is in column B = column index 1 | |
| const cell = `${SHEET_NAME_METADATA}!B${targetIndex + 1}` // add 1 because Google Sheets is 1-indexed | |
| const data = await sheets.spreadsheets.values.get({ | |
| spreadsheetId: SPREADSHEET_ID, | |
| range: cell, | |
| }) | |
| return data?.data?.values?.[0]?.[0] ?? null | |
| } catch (error: any) { | |
| console.error(error) | |
| throw new Error(`Google Sheets API request failed: ${error?.message}`) | |
| } | |
| } | |
| async function sheetsGetMetadataValueByKey (key: string): Promise<string | null> { | |
| try { | |
| const sheets = await getUserSheetsClient() | |
| const metadataValueRange = await sheets.spreadsheets.values.get({ | |
| spreadsheetId: SPREADSHEET_ID, | |
| range: SHEET_NAME_METADATA, // or your sheet name | |
| }) | |
| const rows = metadataValueRange.data.values || [] | |
| const targetIndex = rows.findIndex( | |
| (row) => row[0]?.trim() === key, | |
| ) | |
| if (targetIndex === -1) { | |
| // Key not present in Metadata sheet | |
| return null | |
| } | |
| // The value is in column B = column index 1 | |
| const cell = `${SHEET_NAME_METADATA}!B${targetIndex + 1}` // add 1 because Google Sheets is 1-indexed | |
| const data = await sheets.spreadsheets.values.get({ | |
| spreadsheetId: SPREADSHEET_ID, | |
| range: cell, | |
| }) | |
| return data?.data?.values?.[0]?.[0] ?? null | |
| } catch (error: any) { | |
| console.error(error) | |
| throw new Error(`Google Sheets API request failed for key '${key}': ${error?.message}`) | |
| } | |
| } |
🤖 Prompt for AI Agents
In services/GoogleSheetsService.ts around lines 53-83, the function currently
throws when the key row is not found and the error text hardcodes
'announcement'; update it to match the declared Promise<string | null> by
returning null when targetIndex === -1 (do not throw), and also remove or
replace the hardcoded 'announcement' string so any thrown error (if you keep
any) interpolates the provided key; keep the rest of the logic the same so a
missing key yields null to callers like sheetsGetAnnouncement.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
components/Admin/Announcement/AnnoucementForm.tsx (1)
38-65: Improve submit handler: fix typo, clear error state, and simplify withasync/awaitThe current handler works but has a few rough edges:
- The variable is misspelled as
annoucenment, which makes the code harder to read.erroris never cleared on a new submit, so a previous error message will remain visible even after a successful retry.- The
.then/.catch/.finallychain is a bit noisy given modernasync/await.You can address all three in one pass:
- const handleSubmit = (e: React.FormEvent) => { + const handleSubmit = async (e: React.FormEvent) => { e.preventDefault() - const annoucenment: AnnouncementData = { + const announcement: AnnouncementData = { date, start_time: startTime, end_time: moment(`${date} ${startTime}`). add(duration, 'minutes'). format('HH:mm'), message, car_reg_number: type === 'Car' ? carReg : null, } - console.log('Submitting:', annoucenment) - // Perform your API POST or upload here... - - setIsLoading(true) - createAnnouncement(annoucenment). - then(() => { - console.log('Announcement added successfully') - onComplete(annoucenment) - }). - catch((error) => setError(`Error creating announcement: ${error}`)). - finally(() => { - setIsLoading(false) - }) + console.log('Submitting:', announcement) + + setError(null) + setIsLoading(true) + try { + await createAnnouncement(announcement) + onComplete(announcement) + } catch (error) { + setError(`Error creating announcement: ${error}`) + } finally { + setIsLoading(false) + } }This ensures a clean error state on each submit, fixes the naming typo, and keeps the flow easier to follow.
services/GoogleSheetsService.ts (1)
53-83: AlignsheetsGetMetadataValueByKeybehavior with itsPromise<string | null>return typeRight now:
- The function is typed as
Promise<string | null>, but if the key row is missing it throws instead of returningnull.- Callers like
sheetsGetAnnouncementtherefore can’t distinguish “no announcement set yet” from a genuine API failure.Returning
nullwhen the key is absent better matches the type and the intended “optional metadata” use case, while reserving throws for real API errors. For example:const rows = metadataValueRange.data.values || [] const targetIndex = rows.findIndex( (row) => row[0]?.trim() === key, ) if (targetIndex === -1) { - throw new Error( - `Could not find '${key}' row in sheet ${SHEET_NAME_METADATA}`) + // Key not present in Metadata sheet + return null } @@ } catch (error: any) { console.error(error) - throw new Error(`Google Sheets API request failed: ${error?.message}`) + throw new Error(`Google Sheets API request failed for key '${key}': ${error?.message}`) }That way
sheetsGetAnnouncementcan safely treatnullas “no announcement configured” and only surface hard errors when the Sheets API itself fails.
🧹 Nitpick comments (5)
components/Announcement/AnnouncementModal.tsx (1)
14-28: Optional: Consider reusing the sharedModalfor consistency and a11yThis component largely duplicates the backdrop/layout logic from
components/ui/Modal.tsxbut without ESC handling, title wiring, or a close button. If the behavior is meant to be the same, consider reusingModal(or extracting shared overlay styles) so modal behavior and accessibility stay in sync in one place.components/Admin/Announcement/AddAnnouncement.tsx (1)
3-8: Unify “Announcement” spelling in file/import namesThe form file and import path use
AnnoucementForm(missing the second “n”) while the component itself is namedAnnouncementForm. This inconsistency makes the component harder to find and is easy to mis-type later.Consider renaming the file and updating this import:
-import { AnnouncementForm } from '@/components/Admin/Announcement/AnnoucementForm' +import { AnnouncementForm } from '@/components/Admin/Announcement/AnnouncementForm'…and then
git mv components/Admin/Announcement/AnnoucementForm.tsx components/Admin/Announcement/AnnouncementForm.tsxplus any other imports that reference the old path.README.md (2)
146-158: Add language identifiers to fenced env‑var code blocks (markdownlint MD040)The two env‑var example blocks are fenced with plain ``` which triggers MD040 (
fenced-code-language). Adding a language (e.g. `sh`) will fix the lint and improve rendering:-``` +```sh e.g. ADMIN_GOOGLE_SA_PRIVATE_KEY="-----BEGIN PRIVATE KEY-----\n******\n-----END PRIVATE KEY-----\n" ADMIN_GOOGLE_SA_EMAIL=XXXX@XXXX-XXXX.iam.gserviceaccount.com -``` +``` @@ -``` +```sh AUTH_USERNAME=myuser AUTH_PASSWORD=secret -``` +```
188-189: Nit: Capitalize “Google Sheets” in env var descriptionsIn the env var table, the descriptions for
ADMIN_GOOGLE_SA_PRIVATE_KEYandADMIN_GOOGLE_SA_EMAILsay “your google sheets”. For polish and consistency with earlier sections, consider changing to “your Google Sheets”.-| ADMIN_GOOGLE_SA_PRIVATE_KEY | "-----BEGIN PRIVATE KEY-----\n******\n-----END PRIVATE KEY-----\n" | | Required as part of Admin interface to interact with your google sheets | -| ADMIN_GOOGLE_SA_EMAIL | XXXX@XXXX-XXXX.iam.gserviceaccount.com | | Required as part of Admin interface to interact with your google sheets | +| ADMIN_GOOGLE_SA_PRIVATE_KEY | "-----BEGIN PRIVATE KEY-----\n******\n-----END PRIVATE KEY-----\n" | | Required as part of Admin interface to interact with your Google Sheets | +| ADMIN_GOOGLE_SA_EMAIL | XXXX@XXXX-XXXX.iam.gserviceaccount.com | | Required as part of Admin interface to interact with your Google Sheets |services/GoogleSheetsService.ts (1)
15-37: Consider validating required env vars before constructing the Sheets client
getUserSheetsClientassumesADMIN_GOOGLE_SA_PRIVATE_KEY,ADMIN_GOOGLE_SA_EMAIL, andSPREADSHEET_IDare all set. If any are missing, the JWT or Sheets calls will fail with less-obvious runtime errors.You could fail fast with clearer diagnostics:
export async function getUserSheetsClient () { const session = await getSession() // next-auth v5 app router helper if (!session) { throw new Error('Not authenticated with Google') } + if (!ADMIN_GOOGLE_SA_EMAIL || !ADMIN_GOOGLE_SA_PRIVATE_KEY || !SPREADSHEET_ID) { + throw new Error('Google Sheets service account env vars are not fully configured') + } + try { @@ } catch (err: any) { throw new Error(`Google Service Account error: ${err.message}`) } }This makes deployment misconfigurations much easier to spot.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(8 hunks)app/admin/SessionProviderWrapper.tsx(1 hunks)app/auth.ts(1 hunks)components/Admin/Announcement/AddAnnouncement.tsx(1 hunks)components/Admin/Announcement/AnnoucementForm.tsx(1 hunks)components/Announcement/Announcement.tsx(1 hunks)components/Announcement/AnnouncementModal.tsx(1 hunks)services/GoogleSheetsService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/auth.ts
- components/Announcement/Announcement.tsx
- app/admin/SessionProviderWrapper.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
components/Admin/Announcement/AddAnnouncement.tsx (5)
types/AnnouncementType.d.ts (1)
AnnouncementData(1-10)components/ui/LoadingSpinner.tsx (1)
LoadingSpinner(3-11)components/ui/Buttons.tsx (1)
PrimaryButton(4-24)components/ui/Modal.tsx (1)
Modal(21-91)components/Admin/Announcement/AnnoucementForm.tsx (1)
AnnouncementForm(13-172)
components/Admin/Announcement/AnnoucementForm.tsx (3)
types/AnnouncementType.d.ts (1)
AnnouncementData(1-10)services/MosqueDataService.ts (1)
createAnnouncement(110-112)components/ui/LoadingSpinner.tsx (1)
LoadingSpinner(3-11)
services/GoogleSheetsService.ts (2)
types/AnnouncementType.d.ts (1)
AnnouncementData(1-10)types/DailyPrayerTimeType.d.ts (1)
DailyPrayerTime(1-11)
🪛 LanguageTool
README.md
[style] ~99-~99: Consider using a different verb to strengthen your wording.
Context: ...n generate your own endpoint but please make sure it has all the required properties. ##...
(MAKE_SURE_ENSURE)
[uncategorized] ~188-~188: “Google” is a proper noun and needs to be capitalized.
Context: ...f Admin interface to interact with your google sheets | | ADMIN_GOOGLE_SA_EMAI...
(A_GOOGLE)
[uncategorized] ~189-~189: “Google” is a proper noun and needs to be capitalized.
Context: ...f Admin interface to interact with your google sheets | | AUTH_USERNAME ...
(A_GOOGLE)
🪛 markdownlint-cli2 (0.18.1)
README.md
22-22: Link text should be descriptive
(MD059, descriptive-link-text)
129-129: Bare URL used
(MD034, no-bare-urls)
134-134: Bare URL used
(MD034, no-bare-urls)
146-146: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
155-155: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
179-179: Bare URL used
(MD034, no-bare-urls)
189-189: Bare URL used
(MD034, no-bare-urls)
213-213: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (1)
services/GoogleSheetsService.ts (1)
149-212: Prayer‑times mapping fix looks consistent withDailyPrayerTimenowInitializing
objas{}and explicitly treatingmonth,month_label,day_of_month, andsunrise_startas top‑level fields resolves the earlier runtime error and mis‑mapping. The remaining headers are correctly funneled into nested prayer objects (with the ASR first/second special cases), which aligns with theDailyPrayerTime/PrayerTimeshapes.
| // Set default date = today | ||
| // Default start_time = 2 min from now | ||
| useEffect(() => { | ||
| const now = new Date() | ||
|
|
||
| // Date YYYY-MM-DD | ||
| setDate(now.toISOString().split('T')[0]) | ||
|
|
||
| // Start time = now + 2 minutes | ||
| const start = new Date(now.getTime()) | ||
| setStartTime(start.toISOString().slice(11, 16)) // HH:MM | ||
| }, []) |
There was a problem hiding this comment.
Bug: “2 min from now” default and timezone handling don’t match the implementation
The effect comment says start time should be “now + 2 minutes”, but:
const start = new Date(now.getTime())doesn’t add 2 minutes.- Using
toISOString().slice(11, 16)gives UTC time, so on non‑UTC clients the default time will not match the user’s local clock.
This will produce a surprising default start time for admins.
You can lean on moment (already imported) to both add the 2 minutes and respect local time:
- useEffect(() => {
- const now = new Date()
-
- // Date YYYY-MM-DD
- setDate(now.toISOString().split('T')[0])
-
- // Start time = now + 2 minutes
- const start = new Date(now.getTime())
- setStartTime(start.toISOString().slice(11, 16)) // HH:MM
- }, [])
+ useEffect(() => {
+ const start = moment().add(2, 'minutes')
+
+ setDate(start.format('YYYY-MM-DD'))
+ setStartTime(start.format('HH:mm'))
+ }, [])📝 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.
| // Set default date = today | |
| // Default start_time = 2 min from now | |
| useEffect(() => { | |
| const now = new Date() | |
| // Date YYYY-MM-DD | |
| setDate(now.toISOString().split('T')[0]) | |
| // Start time = now + 2 minutes | |
| const start = new Date(now.getTime()) | |
| setStartTime(start.toISOString().slice(11, 16)) // HH:MM | |
| }, []) | |
| // Set default date = today | |
| // Default start_time = 2 min from now | |
| useEffect(() => { | |
| const start = moment().add(2, 'minutes') | |
| setDate(start.format('YYYY-MM-DD')) | |
| setStartTime(start.format('HH:mm')) | |
| }, []) |
🤖 Prompt for AI Agents
In components/Admin/Announcement/AnnoucementForm.tsx around lines 25 to 36, the
effect that sets the default date/time currently doesn’t add 2 minutes to the
start_time and uses toISOString() which returns UTC, causing incorrect local
times; change the logic to use the existing moment import to get the local date
and time, set date from moment().format('YYYY-MM-DD'), compute start time with
moment().add(2, 'minutes') and format it as moment().format('HH:mm') so the
default is actually now+2 minutes in the user’s local timezone.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
services/GoogleSheetsService.ts (1)
53-83: Fix return type and null-handling inconsistency.Two issues:
- Type mismatch: Line 53 declares
Promise<string>but line 78 can returnnull.- Behavior inconsistency:
sheetsGetAnnouncement(line 42) checks fornull, but this function throws on missing keys (line 66) instead of returningnull, so that check never executes as intended.Based on the caller's expectation and the past review suggestion, update the return type and eliminate the throw for missing keys:
-async function sheetsGetMetadataValueByKey (key: string): Promise<string> { +async function sheetsGetMetadataValueByKey (key: string): Promise<string | null> { try { const sheets = await getUserSheetsClient() const metadataValueRange = await sheets.spreadsheets.values.get({ spreadsheetId: SPREADSHEET_ID, range: SHEET_NAME_METADATA, }) const rows = metadataValueRange.data.values || [] const targetIndex = rows.findIndex( (row) => row[0]?.trim() === key, ) if (targetIndex === -1) { - throw new Error( - `Could not find '${key}' row in sheet ${SHEET_NAME_METADATA}`) + // Key not present in Metadata sheet + return null } // The value is in column B = column index 1 const cell = `${SHEET_NAME_METADATA}!B${targetIndex + 1}` const data = await sheets.spreadsheets.values.get({ spreadsheetId: SPREADSHEET_ID, range: cell, }) return data?.data?.values?.[0]?.[0] ?? null } catch (error: any) { console.error(error) throw new Error(`Google Sheets API request failed: ${error?.message}`) } }
🧹 Nitpick comments (3)
services/GoogleSheetsService.ts (3)
8-10: Validate required environment variables at module load.The service account credentials are required for all operations but aren't validated upfront. If
ADMIN_GOOGLE_SA_PRIVATE_KEYorADMIN_GOOGLE_SA_EMAILare undefined, you'll get obscure JWT errors later. Consider throwing early or logging a clear warning if these are missing.const SPREADSHEET_ID = process.env.SPREADSHEET_ID ?? '' const ADMIN_GOOGLE_SA_PRIVATE_KEY = process.env.ADMIN_GOOGLE_SA_PRIVATE_KEY const ADMIN_GOOGLE_SA_EMAIL = process.env.ADMIN_GOOGLE_SA_EMAIL + +if (!ADMIN_GOOGLE_SA_PRIVATE_KEY || !ADMIN_GOOGLE_SA_EMAIL) { + throw new Error('Missing required environment variables: ADMIN_GOOGLE_SA_PRIVATE_KEY and ADMIN_GOOGLE_SA_EMAIL must be set') +}
85-121: Consider extracting duplicate target-finding logic.Lines 89-101 duplicate the target-row lookup from
sheetsGetMetadataValueByKey(lines 56-68). A small helper would reduce repetition:async function findMetadataRowIndex(sheets: any, key: string): Promise<number> { const metadataValueRange = await sheets.spreadsheets.values.get({ spreadsheetId: SPREADSHEET_ID, range: SHEET_NAME_METADATA, }) const rows = metadataValueRange.data.values || [] const targetIndex = rows.findIndex((row) => row[0]?.trim() === key) if (targetIndex === -1) { throw new Error(`Could not find '${key}' row in sheet ${SHEET_NAME_METADATA}`) } return targetIndex }Then use it in both functions. Since the logic is short, deferring this is fine.
156-157: The@ts-ignorecomment is unnecessary.Line 157 already types
objasany, so TypeScript won't complain. The@ts-ignorecan be removed unless it was addressing a different error.- //@ts-ignore const obj: any = { }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/Admin/Announcement/AnnoucementForm.tsx(1 hunks)components/ui/Buttons.tsx(1 hunks)components/ui/Modal.tsx(1 hunks)services/GoogleSheetsService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/Admin/Announcement/AnnoucementForm.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
services/GoogleSheetsService.ts (3)
app/auth.ts (1)
getSession(82-82)types/AnnouncementType.d.ts (1)
AnnouncementData(1-10)types/DailyPrayerTimeType.d.ts (1)
DailyPrayerTime(1-11)
🔇 Additional comments (7)
components/ui/Buttons.tsx (1)
11-25: LGTM! Children are now rendered correctly.The button implementation is solid. The component properly renders children, forwards all props including
onClick, and includes comprehensive styling for various states (hover, active, focus, disabled). The past issue about missing children has been resolved.components/ui/Modal.tsx (3)
29-33: LGTM! SSR hydration issue properly addressed.The mounted state pattern correctly prevents SSR hydration mismatches. By returning
nulluntil the component is mounted client-side, both server and initial client render produce the same output, then the portal is created only after hydration is complete. This resolves the issue flagged in past reviews.Also applies to: 47-48
56-56: Verify that disabling backdrop-click-to-close is intentional.The backdrop
onClickhandler is commented out (line 56), which means users cannot close the modal by clicking outside of it. They must use the ESC key or the close button. While this prevents accidental dismissal, it differs from common modal UX patterns and may not meet user expectations.If this restriction is intentional for specific dialogs (e.g., critical confirmations), consider:
- Making it configurable via a prop like
closeOnBackdropClick?: boolean- Documenting this behavior for consumers of the Modal component
If accidental, uncomment line 56:
- // onClick={onClose} // click on backdrop closes + onClick={onClose} // click on backdrop closes
36-45: LGTM! ESC key handler implemented correctly.The keyboard event listener properly closes the modal on Escape key press, with correct cleanup and dependencies. The effect only attaches the listener when the modal is open, preventing unnecessary event handling.
services/GoogleSheetsService.ts (3)
15-37: LGTM!The session validation and JWT authentication setup follow best practices for Google Sheets API integration.
48-51: LGTM!Clean wrapper for announcement updates.
159-211: LGTM!The prayer time mapping logic correctly handles top-level keys, ASR special cases, and generic prayer attributes. The fixes from the previous review have been properly applied.
| export function PrimaryButton ({ | ||
| //@ts-ignore | ||
| onClick, | ||
| children, | ||
| ...props | ||
| }: React.ButtonHTMLAttributes<HTMLButtonElement>) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unnecessary //@ts-ignore directive.
The //@ts-ignore on line 5 is unnecessary because React.ButtonHTMLAttributes<HTMLButtonElement> already includes onClick with the correct type (React.MouseEventHandler<HTMLButtonElement> | undefined). Removing this directive will restore TypeScript's type checking without breaking functionality.
Apply this diff:
export function PrimaryButton ({
- //@ts-ignore
onClick,
children,
...props
}: React.ButtonHTMLAttributes<HTMLButtonElement>) {📝 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 PrimaryButton ({ | |
| //@ts-ignore | |
| onClick, | |
| children, | |
| ...props | |
| }: React.ButtonHTMLAttributes<HTMLButtonElement>) { | |
| export function PrimaryButton ({ | |
| onClick, | |
| children, | |
| ...props | |
| }: React.ButtonHTMLAttributes<HTMLButtonElement>) { |
🤖 Prompt for AI Agents
components/ui/Buttons.tsx around lines 4 to 9: remove the unnecessary
"//@ts-ignore" above the onClick prop so TypeScript can type-check props
normally; simply delete that directive and ensure the component signature
remains "export function PrimaryButton({ onClick, children, ...props }:
React.ButtonHTMLAttributes<HTMLButtonElement>)" (no other changes required).
| </div> | ||
| <ServiceWorker /> | ||
| </main> | ||
| <Announcement /> |
There was a problem hiding this comment.
Do you want to make this conditional? Will it break if things aren't set up?
| useEffect(() => { | ||
| const now = new Date() | ||
|
|
||
| // Date YYYY-MM-DD | ||
| setDate(now.toISOString().split('T')[0]) | ||
|
|
||
| const start = new Date(now.getTime()) | ||
| setStartTime(start.toISOString().slice(11, 16)) // HH:MM | ||
| }, []) |
There was a problem hiding this comment.
Feels quite manual - can we use momentjs to get the information we need?
| // Only handle GET requests | ||
| if (e.request.method !== "GET") { | ||
| return; | ||
| } |
There was a problem hiding this comment.
what was the reason we needed to do this?
There was a problem hiding this comment.
without this POST fetch calls were being cached, I assumed the only http call we want to cache is GET
services/GoogleSheetsService.ts
Outdated
| * Converts the PrayerTimes spreadsheet values into the DailyPrayerTime json schema | ||
| * @param values | ||
| */ | ||
| function prayerTimeValuesToPrayerTimesJsonSchema (values: any[][] | null = []): DailyPrayerTime[] { |
There was a problem hiding this comment.
Is there a reason why we needed this?
There was a problem hiding this comment.
I kept this as I ran a POC to fetch prayer times via the sheets, this way we wont need to pass the data to the mosque tech api and the screen can handle it all - can remove for now
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
components/Admin/Announcement/AnnoucementForm.tsx (1)
25-33: Code comment claims "2 min from now" but implementation doesn't add 2 minutes.Line 26 states the default start time should be "2 min from now", but line 32 sets it to the current time without adding 2 minutes. This discrepancy was flagged in a previous review but remains unresolved.
Update the implementation to match the comment's intent:
useEffect(() => { - const now = moment() + const now = moment().add(2, 'minutes') // Date YYYY-MM-DD setDate(now.format('YYYY-MM-DD')) setStartTime(now.format('HH:mm')) }, [])
🧹 Nitpick comments (3)
components/Admin/Announcement/AnnoucementForm.tsx (3)
51-60: Clear previous error state at submission start.When resubmitting after a failed attempt, the previous error message will still be displayed until the new submission completes. Clear the error state when starting a new submission.
+ setError(null) setIsLoading(true) try {
76-76: Replaceas anywith proper type assertion.Using
as anybypasses TypeScript's type checking. Since the value comes from a<select>with only 'General' and 'Car' options, you can safely assert it as the union type.- onChange={(e) => setType(e.target.value as any)} + onChange={(e) => setType(e.target.value as 'General' | 'Car')}
112-119: Consider adding a reasonable maximum duration.Without a
maxattribute, users can enter unreasonably large duration values (e.g., 9999999 minutes). Consider adding a sensible upper bound for better UX and data integrity.<input type="number" min={1} + max={1440} className="w-full rounded-xl border border-slate-300 px-4 py-2 text-sm bg-slate-50 focus:ring-2 focus:ring-mosqueBrand-onPrimary outline-none" value={duration} onChange={(e) => setDuration(Number(e.target.value))} disabled={isLoading} />Note: 1440 minutes = 24 hours. Adjust based on your requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/admin/page.tsx(1 hunks)app/layout.tsx(1 hunks)components/Admin/Announcement/AnnoucementForm.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/admin/page.tsx (6)
app/layout.tsx (1)
metadata(6-15)app/admin/SessionProviderWrapper.tsx (1)
SessionProviderWrapper(10-12)app/auth.ts (3)
isAdminInterfaceEnabled(82-82)getSession(82-82)redirect(56-59)types/MosqueDataType.d.ts (1)
MosqueMetadataType(10-10)services/MosqueDataService.ts (1)
getMetaData(86-90)components/Admin/AdminPage.tsx (1)
AdminPage(5-33)
components/Admin/Announcement/AnnoucementForm.tsx (3)
types/AnnouncementType.d.ts (1)
AnnouncementData(1-10)services/MosqueDataService.ts (1)
createAnnouncement(110-112)components/ui/LoadingSpinner.tsx (1)
LoadingSpinner(3-11)
🔇 Additional comments (3)
app/layout.tsx (1)
8-8: LGTM! Correct Next.js metadata template syntax.The change from
"%"to"%s"aligns with Next.js metadata template conventions for string substitution.Also applies to: 12-12
app/admin/page.tsx (2)
10-13: LGTM! Metadata export is correctly structured.The metadata object will be properly interpolated by the parent layout's
"%s"template.
15-24: LGTM! Clean component structure with proper session context.The SessionProviderWrapper correctly provides NextAuth session context for the admin page.
| redirect("/api/auth/signin") | ||
| } | ||
|
|
||
| const mosqueMetadata: MosqueMetadataType = await getMetaData() |
There was a problem hiding this comment.
Add error handling for metadata fetch.
The getMetaData() call can fail (network errors, API issues, etc.), but there's no error handling. If it throws, the entire admin page will crash with an unhandled exception.
Wrap the fetch in a try-catch block:
+ let mosqueMetadata: MosqueMetadataType
+ try {
+ mosqueMetadata = await getMetaData()
+ } catch (error) {
+ return (
+ <div className="min-h-screen flex items-center justify-center bg-red-50 px-6">
+ <div className="text-center">
+ <h1 className="text-2xl font-semibold mb-2 text-red-700">Error Loading Data</h1>
+ <p className="text-sm text-red-600">Unable to fetch mosque metadata. Please try again later.</p>
+ </div>
+ </div>
+ )
+ }
- const mosqueMetadata: MosqueMetadataType = await getMetaData()
return (📝 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 mosqueMetadata: MosqueMetadataType = await getMetaData() | |
| let mosqueMetadata: MosqueMetadataType | |
| try { | |
| mosqueMetadata = await getMetaData() | |
| } catch (error) { | |
| return ( | |
| <div className="min-h-screen flex items-center justify-center bg-red-50 px-6"> | |
| <div className="text-center"> | |
| <h1 className="text-2xl font-semibold mb-2 text-red-700">Error Loading Data</h1> | |
| <p className="text-sm text-red-600">Unable to fetch mosque metadata. Please try again later.</p> | |
| </div> | |
| </div> | |
| ) | |
| } |
🤖 Prompt for AI Agents
In app/admin/page.tsx around line 50, the direct call const mosqueMetadata:
MosqueMetadataType = await getMetaData() lacks error handling and will crash the
page if getMetaData() throws; wrap the await in a try-catch, catch and log the
error (or surface it to your logger), and provide a safe fallback path—either
set mosqueMetadata to a sensible default/null and render an error state, or
rethrow/return a 500/notFound response after logging, so the page does not crash
on fetch failures.
| <input | ||
| type="text" | ||
| placeholder="" | ||
| className="w-full rounded-xl border border-slate-300 px-4 py-2 text-sm bg-slate-50 focus:ring-2 focus:ring-mosqueBrand-onPrimary outline-none" | ||
| value={carReg} | ||
| onChange={(e) => setCarReg(e.target.value)} | ||
| disabled={isLoading} | ||
| /> |
There was a problem hiding this comment.
Add validation for required fields.
The form allows submission with empty message and empty carReg (when type is 'Car'). This creates announcements with missing critical information.
Add required attributes and validation:
<input
type="text"
placeholder=""
className="w-full rounded-xl border border-slate-300 px-4 py-2 text-sm bg-slate-50 focus:ring-2 focus:ring-mosqueBrand-onPrimary outline-none"
value={carReg}
onChange={(e) => setCarReg(e.target.value)}
disabled={isLoading}
+ required
/> <textarea
rows={3}
className="w-full rounded-xl border border-slate-300 px-4 py-2 text-sm bg-slate-50 focus:ring-2 focus:ring-mosqueBrand-onPrimary outline-none"
value={message}
onChange={(e) => setMessage(e.target.value)}
disabled={isLoading}
+ required
/>Also applies to: 141-147
🤖 Prompt for AI Agents
In components/Admin/Announcement/AnnoucementForm.tsx around lines 127-134 (and
similarly 141-147), the input fields allow submission when required values are
empty; add client-side validation by marking the relevant inputs as required and
preventing form submit when invalid: add required attribute to the message
textarea and to the carReg input (or conditionally required when type ===
'Car'), validate in the form submit handler to check message.trim() is non-empty
and if type === 'Car' ensure carReg.trim() is non-empty, set and display a local
error state for each field and do not call the submit API when validation fails
(focus the first invalid field and optionally set an isLoading flag only after
validation passes).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
components/Admin/Announcement/AnnoucementForm.tsx (2)
128-135: Addrequiredattribute to car registration input.When the announcement type is 'Car', the car registration number is critical information but the input field doesn't have a
requiredattribute. This allows empty car registration submissions, creating incomplete announcements.Apply this diff:
<input type="text" placeholder="" className="w-full rounded-xl border border-slate-300 px-4 py-2 text-sm bg-slate-50 focus:ring-2 focus:ring-mosqueBrand-onPrimary outline-none" value={carReg} onChange={(e) => setCarReg(e.target.value)} disabled={isLoading} + required />
25-33: Implement the "+2 minutes" default as stated in the comment.The comment says "Default start_time = 2 min from now" but the code sets
startTimeto the current time without adding 2 minutes. This creates a mismatch between the stated intent and actual behavior.Apply this diff to add the 2-minute offset:
useEffect(() => { - const now = moment() + const start = moment().add(2, 'minutes') // Date YYYY-MM-DD - setDate(now.format('YYYY-MM-DD')) - setStartTime(now.format('HH:mm')) + setDate(start.format('YYYY-MM-DD')) + setStartTime(start.format('HH:mm')) }, [])
🧹 Nitpick comments (2)
components/Admin/Announcement/AnnoucementForm.tsx (2)
76-76: Replaceas anywith proper type assertion.Using
as anybypasses TypeScript's type checking. Since the type is a strict union of'General' | 'Car', you can safely assert to that type instead.Apply this diff:
- onChange={(e) => setType(e.target.value as any)} + onChange={(e) => setType(e.target.value as 'General' | 'Car')}
56-57: Improve error message formatting.Direct string interpolation of the error object may produce unhelpful messages like "Error creating announcement: [object Object]". Extract the error message properly to provide better feedback to the user.
Apply this diff:
- } catch(error) { - setError(`Error creating announcement: ${error}`) + } catch (error) { + const errorMessage = error instanceof Error ? error.message : String(error) + setError(`Error creating announcement: ${errorMessage}`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/Admin/Announcement/AnnoucementForm.tsx(1 hunks)
🔇 Additional comments (1)
components/Admin/Announcement/AnnoucementForm.tsx (1)
35-62: Excellent fix for async handling!The
handleSubmitfunction now properly awaitscreateAnnouncementand only callsonCompleteafter successful submission. This ensures the modal doesn't close prematurely and prevents state updates on unmounted components.
… where the data will be stored as key value as flattened json This change will allow data to be easily readable and editable in sheets as well as giving the flexibility to store complex data. - Added jest for testing the logic of converting json to rows and rows to json
|
New configuration sheet: |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
jest.config.js (1)
1-17: Config is solid; consider preset spreading and DOM-test environment strategyThis setup is correct for ts-jest and the jose ESM edge case; nothing blocking here. Two optional refinements to consider:
- Spread the whole ts-jest preset, not just
transform
This makes the config more future-proof ifcreateDefaultPreset()ever adds more defaults thantransform:-const tsJestTransformCfg = createDefaultPreset().transform; +const tsJestPreset = createDefaultPreset(); /** @type {import("jest").Config} **/ module.exports = { - testEnvironment: "node", - transform: { - ...tsJestTransformCfg, - }, + ...tsJestPreset, + testEnvironment: "node", moduleNameMapper: { '^@/(.*)$': '<rootDir>/$1', }, transformIgnorePatterns: [ 'node_modules/(?!(jose)/)', ], };
- Double‑check
testEnvironment: "node"vs future React/DOM tests
For server-side / NextAuth / jose-heavy tests,"node"is a good choice and often avoids jose + jsdom issues. If you later add React Testing Library / DOM-based component tests, you may want either:
- A separate Jest config using
testEnvironment: "jsdom"for UI tests, or- Per-test overrides via
/** @jest-environment jsdom */at the top of those files.services/GoogleSheetsUtil.test.ts (1)
1-49: Consider adding test coverage forsheetsUtilValuesToJson.The test suite imports and tests
sheetsUtilFlattenedJsonToRowsandsheetsUtilValuesToNestedJson, butsheetsUtilValuesToJsonis also imported and exported fromGoogleSheetsUtil.ts(line 8 in GoogleSheetsUtil.ts). Adding test coverage for this function would ensure all public utilities are validated.types/MosqueDataType.d.ts (1)
17-17: Consider narrowingConfigurationJsontype if possible.The type
ConfigurationJson = { [key: string]: any }is very permissive. If the structure is known (e.g., it containsannouncement?: AnnouncementData), consider defining a more specific interface to improve type safety and catch errors at compile time.services/GoogleSheetsUtil.ts (1)
38-55: Consider validating keys before processing.The function doesn't validate whether
keyis empty or contains only dots, which could lead to unexpected behavior. Consider adding validation:const key = String(rawKey); +if (!key || key.trim() === '') continue; + const parts = key.split("."); // Split on dots to handle nested keys +if (parts.some(p => !p)) continue; // Skip if any part is emptyservices/GoogleSheetsService.ts (2)
19-41: Consider clarifying the session requirement for service account operations.The function checks for a user session (line 20) but then authenticates with a Google Service Account using JWT (lines 28-32). Service Accounts don't require user sessions—they use their own credentials.
If the intent is to restrict access to authenticated admin users only, this makes sense. However, consider renaming the function to better reflect its purpose (e.g.,
getAuthenticatedSheetsClient) and adding a comment explaining that the session check is for authorization, not for Google Sheets authentication.
67-82: Consider extracting visibility computation to avoid duplication.The visibility computation logic (lines 74-78) is duplicated in
services/MosqueDataService.ts(lines 102-106). Consider extracting this into a shared utility function to maintain consistency and reduce duplication.Example:
// Create a new utility function function computeAnnouncementVisibility(announcement: AnnouncementData): boolean { if (!announcement.date || !announcement.start_time || !announcement.end_time) { return false } const now = moment() return ( now.isSame(announcement.date, 'day') && now.isSameOrAfter(`${announcement.date} ${announcement.start_time}`, 'minutes') && now.isBefore(`${announcement.date} ${announcement.end_time}`, 'minutes') ) }package.json (1)
20-20: Consider updating googleapis to version 166.0.0.The installed version 105.0.0 is significantly outdated (latest stable is 166.0.0 as of December 2025). While no known security vulnerabilities were found in the 105.x branch, updating to the latest version is recommended to benefit from bug fixes, performance improvements, and security patches accumulated across 61 major releases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
app/api/data/announcements/route.ts(1 hunks)jest.config.js(1 hunks)package.json(2 hunks)services/GoogleSheetsService.ts(1 hunks)services/GoogleSheetsUtil.test.ts(1 hunks)services/GoogleSheetsUtil.ts(1 hunks)services/MosqueDataService.ts(2 hunks)tsconfig.json(2 hunks)types/MosqueDataType.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tsconfig.json
- app/api/data/announcements/route.ts
🧰 Additional context used
🧬 Code graph analysis (2)
services/MosqueDataService.ts (2)
types/AnnouncementType.d.ts (1)
AnnouncementData(1-10)services/GoogleSheetsService.ts (1)
sheetsUpdateAnnouncement(84-88)
services/GoogleSheetsUtil.test.ts (1)
services/GoogleSheetsUtil.ts (2)
sheetsUtilFlattenedJsonToRows(84-101)sheetsUtilValuesToNestedJson(25-59)
🔇 Additional comments (8)
services/GoogleSheetsUtil.ts (2)
8-23: LGTM!The function correctly handles empty arrays, uses the first row as headers, and safely handles missing values with the nullish coalescing operator.
84-101: Consider documenting array handling behavior in JSDoc.The implementation correctly flattens nested objects by recursively walking the structure. Arrays will be treated as objects with numeric keys (e.g.,
arr[0],arr[1]). If this is the intended behavior, document it in the JSDoc. If arrays should be handled differently, update the implementation accordingly.services/MosqueDataService.ts (2)
92-109: Verify handling of missing time fields.The visibility computation (lines 102-106) assumes
start_timeandend_timeare present. If these can benullorundefined(as suggested by theAnnouncementDatatype which marks some fields as optional), the string concatenation on lines 104-105 could produce invalid date-time strings like"2025-12-05 undefined", leading to incorrect visibility calculations.Check the AnnouncementData type definition and consider adding guards:
+ if (!announcement.start_time || !announcement.end_time) { + announcement.is_visible = false + return announcement + } + announcement.is_visible = ( now.isSame(announcement?.date, 'day') && now.isSameOrAfter(`${announcement?.date} ${announcement?.start_time}`, 'minutes') && now.isBefore(`${announcement?.date} ${announcement?.end_time}`, 'minutes') )
111-113: LGTM!Clean delegation to the Google Sheets service. Input validation is appropriately handled at other layers.
services/GoogleSheetsService.ts (4)
43-51: LGTM!Clean readiness check with appropriate error handling and logging.
53-65: LGTM!The function correctly fetches and transforms the configuration data with appropriate error handling.
84-88: LGTM!Clean implementation that correctly updates the announcement within the configuration data.
90-102: LGTM!The function correctly flattens the configuration data and updates the sheet with appropriate options.
| "moment": "^2.29.4", | ||
| "moment-hijri": "^2.1.2", | ||
| "next": "^15.4.1", | ||
| "next-auth": "^4.24.13", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
next-auth 4.24.13 Next.js 15.4.1 compatibility
💡 Result:
Short answer: Possibly, but not fully guaranteed — next-auth 4.24.13 contains several backports to improve compatibility with Next.js 14/15, but active issues remain for Next.js 15 (App Router / React 19) and some users report runtime/type errors. Test carefully or use v5 (Auth.js) which targets Next 15.
Sources:
- next-auth GitHub issues reporting Next.js 15/App Router problems and fixes/backports. [1]
- Security/npm page showing latest 4.24.13 release. [2]
Recommendations:
- If you must run Next.js 15 in production: test end-to-end (app router route handlers, server components, React 19) — if you hit issues, consider upgrading to Auth.js (next-auth v5 / auth.js) which has explicit Next 15 support, or pin Next/React versions (e.g., React 18) until fixes land.
🌐 Web query:
next-auth v5 release status stable beta
💡 Result:
Short answer: As of Dec 5, 2025, NextAuth (Auth.js) v5 is still published as beta — maintainers continue releasing 5.0.0-beta.x versions while v4 remains the designated stable line. [1][2]
Sources:
- Release listing showing 5.0.0-beta releases (latest beta entries). [1]
- GitHub discussion/issues noting v5’s beta status and docs request. [2]
🌐 Web query:
next-auth version requirements Next.js support matrix
💡 Result:
Short compatibility matrix (community/maintainer sources):
-
next-auth v4 (stable): officially tested/peer range historically: Next.js ^12.2.5 || ^13 (and later patched to include 14/15 in point releases). See v4 docs/releases and related GitHub issues. [1][2]
-
next-auth v5 / "next-auth" (beta; rebranded toward Auth.js): minimum Next.js noted in upgrade guides/beta docs is Next.js 14.0+. v5 was released as beta/early-stable for Next.js 14+; ecosystem issues show peer-range gaps for Next.js 16 as of late 2025. If you need production stability, v4 is recommended until v5 reaches GA for your Next version. [3][4]
Notes / practical advice:
- Check the package peerDependencies before installing (npm/yarn will show conflicts). [1][4]
- For cutting-edge Next.js (15/16+), you may need beta/updated next-auth (or --legacy-peer-deps) until maintainers add explicit support; follow the next-auth GitHub for PRs/releases. [1][3]
Sources:
- NextAuth.js docs / upgrade notes (v4). [turn0search4] (NextAuth.js upgrade guide).
- npm package / release info for next-auth (shows v4 releases). [turn1search0] (npm next-auth).
- NextAuth v5 upgrade / community guides noting Next.js 14+ minimum for v5. [turn0search7] (v5 upgrade guide).
- GitHub issues about peerDependency/compatibility (Next.js 14–16 compatibility discussions). [turn1search3] (next-auth GitHub issue).
Address next-auth version compatibility with Next.js 15.4.1.
next-auth v4.24.13 has known compatibility issues with Next.js 15.4.1, including App Router and React 19 problems with active unresolved issues on GitHub. While point releases added backports, stability is not guaranteed. next-auth v5 (Auth.js) explicitly targets Next.js 14+ and provides better Next.js 15 support, but remains in beta. Consider either testing the current setup thoroughly across App Router route handlers and server components, or evaluating v5 despite its beta status if Next.js 15 stability is required.
| export async function sheetsGetAnnouncement(): Promise<AnnouncementData | null> { | ||
| const data = await sheetsGetConfigurationData() | ||
| let announcement = data?.announcement as unknown as AnnouncementData ?? 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 | ||
| } |
There was a problem hiding this comment.
Fix null handling before accessing announcement properties.
Line 69 assigns announcement which can be null, but line 74-78 accesses announcement.date, announcement.start_time, and announcement.end_time without null checks. This will cause a runtime error if the announcement is null.
export async function sheetsGetAnnouncement(): Promise<AnnouncementData | null> {
const data = await sheetsGetConfigurationData()
let announcement = data?.announcement as unknown as AnnouncementData ?? null
+ if (!announcement || !announcement.date) {
+ return null
+ }
+
+ if (!announcement.start_time || !announcement.end_time) {
+ announcement.is_visible = false
+ return announcement
+ }
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
}📝 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 async function sheetsGetAnnouncement(): Promise<AnnouncementData | null> { | |
| const data = await sheetsGetConfigurationData() | |
| let announcement = data?.announcement as unknown as AnnouncementData ?? 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 | |
| } | |
| export async function sheetsGetAnnouncement(): Promise<AnnouncementData | null> { | |
| const data = await sheetsGetConfigurationData() | |
| let announcement = data?.announcement as unknown as AnnouncementData ?? null | |
| if (!announcement || !announcement.date) { | |
| return null | |
| } | |
| if (!announcement.start_time || !announcement.end_time) { | |
| announcement.is_visible = false | |
| return announcement | |
| } | |
| 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 | |
| } |
🤖 Prompt for AI Agents
In services/GoogleSheetsService.ts around lines 67 to 82, announcement can be
null but the code unconditionally accesses announcement.date,
announcement.start_time and announcement.end_time; add a null guard before
computing is_visible (for example return null or set announcement.is_visible =
false when announcement is null) or early-return when announcement is null so
you never access properties on a null value; ensure subsequent code handles the
null return consistently.
| // Skip header row if present | ||
| if (rawKey === "key") continue; |
There was a problem hiding this comment.
Header detection may skip legitimate data rows.
The check if (rawKey === "key") continue; skips rows where the first column is exactly "key". If a legitimate configuration entry has "key" as its key name, it will be silently ignored. Consider using a more robust header detection mechanism (e.g., checking if this is the first row or using a special marker).
🤖 Prompt for AI Agents
In services/GoogleSheetsUtil.ts around lines 35-36, the current header detection
unconditionally skips any row whose first cell equals the string "key", which
can drop legitimate entries; instead only treat the first sheet row as a header
(or detect header by matching a known header row shape), so change the logic to
skip the header based on row index (e.g., skip when rowIndex === 0) or compare
the entire row to the expected header array before continuing, ensuring
legitimate rows with "key" as a value are not ignored.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
2 participants
This change adds a new Admin Page to the app, which currently as of this PR allows showing an announcement on the screen.
Admin Page requires username/password authentication which is defined as ENV vars to keep the setup simple. Google Cloud project service account needs to be setup for the backend to be able to edit the sheet data.
Non breaking change - existing screens are not impacted.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.