refactor: split monolithic server.js into route modules and services (#217)#236
Open
rishab11250 wants to merge 3 commits into
Open
refactor: split monolithic server.js into route modules and services (#217)#236rishab11250 wants to merge 3 commits into
rishab11250 wants to merge 3 commits into
Conversation
|
@rishab11250 is attempting to deploy a commit to the xthxr's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughSplits the monolithic server into modular env/config, cached Firebase state, middleware, utilities, an in-memory fallback, and multiple route modules implementing links, analytics, bio, profile, admin, import, split-tests, bug reports, pages, and tracking with consistent error and DB-fallback handling. ChangesServer refactoring into modular architecture
Sequence DiagramssequenceDiagram
participant Client
participant Server
participant Firestore
participant Redis
participant RedirectCache
Client->>Server: POST /api/shorten (url, optional custom code)
Server->>Server: Validate URL and customShortCode
Server->>Firestore: Check shortCode availability
Firestore-->>Server: availability result
Server->>Server: generate shortCode / apply UTM / health check
Server->>Firestore: write link + analytics documents
Firestore-->>Server: write success
Server->>Redis: cache redirect mapping
Redis-->>Server: cached
Server->>RedirectCache: update in-memory redirect cache
RedirectCache-->>Server: cached
Server-->>Client: 201 { shortUrl, shortCode }
sequenceDiagram
participant User
participant Server
participant verifyToken
participant FirebaseAuth
participant Firestore
User->>Server: GET /api/user/links with Bearer token
Server->>verifyToken: run middleware
verifyToken->>FirebaseAuth: verify ID token
FirebaseAuth-->>verifyToken: decoded uid
verifyToken->>Server: attach req.user
Server->>Firestore: query LINKS by userId
Firestore-->>Server: link docs
Server->>Server: auto-expire/deactivate, aggregate analytics via getAggregatedAnalytics
Server-->>User: { links: [...] }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (14)
src/routes/profile.js-3-3 (1)
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused import
getFirebaseState.ESLint flagged this as unused.
Proposed fix
-const { getDatabase, getFirebaseState, COLLECTIONS, admin } = require('../../config/firebase.config'); +const { getDatabase, COLLECTIONS, admin } = require('../../config/firebase.config');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/profile.js` at line 3, Remove the unused getFirebaseState import from the destructured require on the top of the module (the line that currently destructures getDatabase, getFirebaseState, COLLECTIONS, admin); modify the destructuring to only include used symbols (getDatabase, COLLECTIONS, admin) and run lint/tests to ensure nothing else in profile.js references getFirebaseState before committing.Source: Pipeline failures
src/routes/analytics.js-3-3 (1)
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused import
getFirebaseState.ESLint flagged this as unused.
Proposed fix
-const { getDatabase, getFirebaseState, COLLECTIONS } = require('../../config/firebase.config'); +const { getDatabase, COLLECTIONS } = require('../../config/firebase.config');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/analytics.js` at line 3, The import line currently destructures getFirebaseState but it's unused; update the require statement to remove getFirebaseState from the destructured symbols (leave getDatabase and COLLECTIONS) and ensure there are no remaining references to getFirebaseState elsewhere in this module (if there are, either remove those references or reintroduce a proper usage). This fixes the unused-import ESLint warning for getFirebaseState.Source: Pipeline failures
src/routes/links.js-3-3 (1)
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused import
getFirebaseState.ESLint flagged this as unused. Remove it from the destructuring to fix the linter warning.
Proposed fix
-const { getDatabase, getFirebaseState, COLLECTIONS, admin } = require('../../config/firebase.config'); +const { getDatabase, COLLECTIONS, admin } = require('../../config/firebase.config');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/links.js` at line 3, Remove the unused getFirebaseState identifier from the destructuring require statement in src/routes/links.js; update the const declaration that currently reads "const { getDatabase, getFirebaseState, COLLECTIONS, admin } = require(...)" to only extract the used symbols (getDatabase, COLLECTIONS, admin) so the linter warning is resolved.Source: Pipeline failures
src/routes/links.js-27-28 (1)
27-28:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrefix unused exception variable with underscore.
ESLint flagged
eas unused. Prefix with underscore to indicate intentional discard.Proposed fix
- } catch (e) { + } catch (_e) { return res.status(400).json({ error: 'Invalid URL' });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/links.js` around lines 27 - 28, The catch block in the links route currently uses an unused exception variable `e`; rename it to `_e` (or `_`) in the catch clause to satisfy ESLint's unused-variable rule (e.g., change `catch (e)` to `catch (_e)` in the route handler in src/routes/links.js) so the error is intentionally discarded while leaving the existing `return res.status(400).json({ error: 'Invalid URL' });` behavior unchanged.Source: Pipeline failures
src/routes/bug-report.js-17-17 (1)
17-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify fallback behavior in user-facing message.
The success message "Bug report received (logged locally)" implies the report was captured and will be processed, but it is only logged to the console without persistence or guaranteed follow-up. This is misleading to users who expect their feedback to be stored and reviewed. Consider either implementing a persistent fallback (e.g., file-based log, message queue) or revising the message to set accurate expectations.
💬 Suggested message revision
- return res.json({ success: true, message: 'Bug report received (logged locally)' }); + return res.json({ + success: false, + message: 'Bug reporting is temporarily unavailable. Please try again later.' + });Or, if console logging is acceptable as a fallback, be explicit:
- return res.json({ success: true, message: 'Bug report received (logged locally)' }); + return res.json({ + success: true, + message: 'Bug report logged. Due to temporary system unavailability, manual follow-up may be required.' + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/bug-report.js` at line 17, The user-facing response in the bug report route currently returns res.json({ success: true, message: 'Bug report received (logged locally)' }) while only logging to console; either make that behavior explicit in the message or add a persistent fallback. In the POST /bug-report handler (the route handler that calls console.log and returns res.json), either 1) change the message to clearly state non-persistence and no guaranteed follow-up (e.g., "Bug report received — logged locally; no persistence or guaranteed follow-up"), or 2) implement simple persistence (e.g., append the report to a durable file via fs.appendFile or push it to your existing queue/DB) and then update the res.json message to reflect that the report was persisted; update references to the handler/res.json and any console.log usage accordingly.src/routes/bio.js-3-3 (1)
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused import
getFirebaseState.The ESLint warning indicates
getFirebaseStateis imported but never used in this file.🔧 Proposed fix
-const { getDatabase, getFirebaseState, COLLECTIONS, admin } = require('../../config/firebase.config'); +const { getDatabase, COLLECTIONS, admin } = require('../../config/firebase.config');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/bio.js` at line 3, The import destructures getFirebaseState but it isn't used; remove getFirebaseState from the require statement so the line reads only include getDatabase, COLLECTIONS, and admin (i.e., update the destructuring in the require on that line to remove getFirebaseState) and confirm there are no other references to getFirebaseState in this file.Source: Pipeline failures
src/routes/admin.js-3-3 (1)
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused import
admin.The ESLint warning indicates
adminis imported but never used in this file.🔧 Proposed fix
-const { getDatabase, getFirebaseState, COLLECTIONS, admin } = require('../../config/firebase.config'); +const { getDatabase, getFirebaseState, COLLECTIONS } = require('../../config/firebase.config');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/admin.js` at line 3, Remove the unused destructured symbol "admin" from the require statement so only the used exports (getDatabase, getFirebaseState, COLLECTIONS) are imported; update the require(...) line that currently destructures { getDatabase, getFirebaseState, COLLECTIONS, admin } to exclude admin and save the file to clear the ESLint unused-import warning.Source: Pipeline failures
src/routes/bio.js-198-201 (1)
198-201:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
COLLECTIONS.BIO_LINKSconstant instead of hardcoded string.Line 198 uses the hardcoded string
'bioLinks'while other endpoints in this file useCOLLECTIONS.BIO_LINKS. This inconsistency could cause issues if the collection name is ever changed in the config.🔧 Proposed fix
- const bioLinksSnapshot = await db.collection('bioLinks') + const bioLinksSnapshot = await db.collection(COLLECTIONS.BIO_LINKS)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/bio.js` around lines 198 - 201, Replace the hardcoded collection name in the db query with the shared constant: change the db.collection('bioLinks') call that creates bioLinksSnapshot to use COLLECTIONS.BIO_LINKS instead so it matches other endpoints and centralizes the collection name; update the line that builds bioLinksSnapshot (the db.collection(...) .where(...).limit(1).get() chain) to reference COLLECTIONS.BIO_LINKS.src/routes/import.js-5-5 (1)
5-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused import
toFirestoreId.The import is flagged by ESLint and is not used anywhere in this file.
🔧 Proposed fix
-const { generateShortCode, toFirestoreId } = require('../utils/url.utils'); +const { generateShortCode } = require('../utils/url.utils');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/import.js` at line 5, The import line currently destructures both generateShortCode and toFirestoreId from '../utils/url.utils' but toFirestoreId is unused; update the require statement to only destructure generateShortCode (remove toFirestoreId), verify there are no remaining references to toFirestoreId in this module (functions/handlers in src/routes/import.js) and run ESLint to confirm the warning is resolved.Source: Pipeline failures
src/routes/split-test.js-34-34 (1)
34-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWeight values are not sanitized.
v.weight || 50accepts any truthy value including negative numbers, strings, orNaN, which could produce unexpected split-test behavior. Consider coercing to a positive number.🔧 Suggested improvement
- splitTest: { variants: variants.map(v => ({ url: v.url, weight: v.weight || 50, name: v.name || '' })) } + splitTest: { + variants: variants.map(v => ({ + url: v.url, + weight: Math.max(0, Number(v.weight) || 50), + name: String(v.name || '') + })) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/split-test.js` at line 34, The weight mapping for splitTest currently uses v.weight || 50 which accepts non-numeric or negative values; update the mapping in splitTest so the weight is coerced to a number and clamped to a valid positive range (e.g., parse Number(v.weight), fallback to 50 when not finite, then clamp/round to a reasonable range like 0–100) when building variants (reference the splitTest object, variants.map and v.weight).config/firebase.config.js-46-50 (1)
46-50:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove or use the
errorvariable to fix the linter warning.The catch block captures
errorbut never uses it. Either log the error for debugging or replace with_to indicate intentional discard.🔧 Proposed fix
- } catch (error) { + } catch { console.log('⚠️ Firebase Admin not configured. Using in-memory storage.'); console.log(' See docs/FIREBASE_SETUP.md for setup instructions.'); return false;Or if you want to log the actual error for debugging:
} catch (error) { console.log('⚠️ Firebase Admin not configured. Using in-memory storage.'); console.log(' See docs/FIREBASE_SETUP.md for setup instructions.'); + console.log(' Error:', error.message); return false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/firebase.config.js` around lines 46 - 50, The catch block in config/firebase.config.js currently captures error but never uses it; either log the error inside the catch (e.g., call console.error with a descriptive message plus the error) to preserve debugging info when Firebase Admin fails to configure, or if the error is intentionally ignored rename the parameter to _ (catch (_)) to satisfy the linter; update the catch around the Firebase init logic accordingly (the existing catch(error) shown in the diff).Source: Pipeline failures
src/routes/tracking.js-247-248 (1)
247-248:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unnecessary escape characters in regex.
Inside a character class,
/and[do not require escaping. The linter flags these as unnecessary.🧹 Proposed fix
- [`browsers.${browserType.replace(/[.#$\/\[\]]/g, '_')}`]: admin.firestore.FieldValue.increment(1), - [`referrers.${referrerSource.replace(/[.#$\/\[\]]/g, '_')}`]: admin.firestore.FieldValue.increment(1), + [`browsers.${browserType.replace(/[.#$/[\]]/g, '_')}`]: admin.firestore.FieldValue.increment(1), + [`referrers.${referrerSource.replace(/[.#$/[\]]/g, '_')}`]: admin.firestore.FieldValue.increment(1),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/tracking.js` around lines 247 - 248, The character-class regex /[.#$\/\[\]]/g escapes / and [ unnecessarily; update both replace calls (browserType.replace and referrerSource.replace) to use a simplified class without those extra backslashes, e.g. /[.#$[\]]/g, so you remove the unnecessary escapes while still handling the literal ] correctly.src/routes/pages.js-3-3 (1)
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused imports.
The
getDatabaseandCOLLECTIONSimports are not used in this file.🧹 Proposed fix
-const { getDatabase, COLLECTIONS } = require('../../config/firebase.config');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/pages.js` at line 3, The require line imports unused symbols; remove getDatabase and COLLECTIONS from the destructuring require (the line that currently reads "const { getDatabase, COLLECTIONS } = require('../../config/firebase.config');") or replace it with only the exports actually used in this module so there are no unused imports; ensure no other references to getDatabase or COLLECTIONS remain in the file after the change.server.js-102-102 (1)
102-102:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused import.
The
memoryStoreimport is not used in the expiry notification interval.🧹 Proposed fix
- const memoryStore = require('./src/services/memory.service');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server.js` at line 102, Remove the unused import by deleting the const memoryStore = require('./src/services/memory.service'); line in server.js since memoryStore is not referenced by the expiry notification interval or anywhere else in this file; ensure no other code depends on memoryStore and run tests/lint to confirm no remaining references.
🧹 Nitpick comments (10)
src/routes/links.js (1)
31-37: 💤 Low valueRedundant scheme check after URL parsing validation.
Lines 23-26 already validate that only
http:andhttps:protocols are allowed. TheblockedSchemescheck forjavascript:,data:,vbscript:can never trigger since those would have been rejected above.Proposed removal
- const blockedSchemes = ['javascript:', 'data:', 'vbscript:']; - const urlLower = url.toLowerCase(); - for (const scheme of blockedSchemes) { - if (urlLower.startsWith(scheme)) { - return res.status(400).json({ error: 'Invalid URL: dangerous URL scheme blocked' }); - } - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/links.js` around lines 31 - 37, Remove the redundant blockedSchemes loop: since the prior URL protocol validation already permits only "http:" and "https:", delete the blockedSchemes array and the for-loop that checks urlLower.startsWith(scheme) (the variables blockedSchemes and urlLower and that conditional return). This simplifies the route handler (links.js) and avoids unreachable checks while preserving the existing protocol validation logic.src/routes/analytics.js (1)
53-70: 💤 Low valueNPE risk when
dbis null.If
getDatabase()returnsnull, callingdb.collection()on line 57 throws aTypeErrorrather than a Firestore error. This technically falls through to the in-memory fallback via the catch block, but the error message "Error reading from Firestore" is misleading. Consider adding an explicit null check for clarity and consistent error messaging.Proposed improvement
try { + const db = getDatabase(); + if (!db) throw new Error('Firestore unavailable'); + const firestoreId = toFirestoreId(shortCode); - const linkDoc = await db.collection(COLLECTIONS.LINKS).doc(firestoreId).get(); + const linkDoc = await db.collection(COLLECTIONS.LINKS).doc(firestoreId).get();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/analytics.js` around lines 53 - 70, getDatabase() can return null which causes a TypeError when calling db.collection(...); add an explicit null check immediately after const db = getDatabase() and handle the Firestore-unavailable case with a clear log and the same in-memory fallback or response used elsewhere (do not let the TypeError fall into the generic Firestore catch). Specifically, in the route containing toFirestoreId(shortCode), COLLECTIONS.LINKS lookup and getAggregatedAnalytics(firestoreId), check if (!db) then call the existing in-memory analytics path or return an appropriate response (and log "Firestore not available" or similar) before attempting db.collection() so the error message and control flow remain correct.src/routes/bug-report.js (1)
9-11: ⚡ Quick winAdd email format validation.
The validation only checks for the presence of the email field but does not verify its format. Consider adding a basic email format check (e.g., regex pattern) to reject malformed addresses before storage.
📧 Suggested email validation
+const EMAIL_REGEX = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + router.post('/api/bug-report', async (req, res) => { const { name, email, subject, message, type } = req.body; if (!name || !email || !message) { return res.status(400).json({ error: 'Name, email, and message are required' }); } + + if (!EMAIL_REGEX.test(email)) { + return res.status(400).json({ error: 'Invalid email format' }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/bug-report.js` around lines 9 - 11, Add a basic email format check after the existing presence checks in the bug report route handler: validate the email string (variable email) against a simple regex (RFC-lite) and if it fails return res.status(400).json({ error: 'Invalid email format' });. Update the conditional that currently checks !name || !email || !message to either include a separate branch for format validation or add an if block using the regex; reference the variables name, email, message and the Express response object res when implementing the validation and error response.src/routes/admin.js (3)
52-77: ⚖️ Poor tradeoffLarge collection fetch may cause memory/timeout issues.
db.collection(COLLECTIONS.LINKS).get()fetches all documents into memory. For a large number of links, this could cause memory pressure or request timeouts.Consider paginating with cursor-based iteration using
startAfter()or streaming withstream()if the links collection grows significantly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/admin.js` around lines 52 - 77, The current code loads the entire collection via db.collection(COLLECTIONS.LINKS).get() into linksSnapshot which can OOM/timeout for large collections; change the sync to a paginated cursor loop (use a query with .limit(pageSize) and .startAfter(lastDoc)) or use Firestore streaming, iterating pages and processing each doc with the existing logic that calls redisUtils.storeLinkInRedis and redirectCache.set (preserve synced and errors counters and the per-doc try/catch). Ensure you update lastDoc after each page and continue until a page returns no documents to avoid loading all docs at once.
86-147: ⚖️ Poor tradeoffConsider cleaning up Redis/redirect cache for deleted links.
When deleting user links, the corresponding entries in Redis (
link:{shortCode}) and redirect cache (redirect-cache:{shortCode}) are not removed. This could cause stale redirects to remain active until their TTL expires.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/admin.js` around lines 86 - 147, The deletion flow in router.delete('/api/user/account', verifyToken) omits removing Redis/redirect-cache entries for each deleted link; update the link deletion loop (where linksSnapshot is iterated and toFirestoreId(linkData.shortCode) is used) to also delete the Redis keys for that shortCode (e.g., `link:{shortCode}` and `redirect-cache:{shortCode}`) — obtain the Redis client from your existing cache module, prefer batching via pipeline/multi when deleting many keys to avoid round-trips, and ensure these Redis deletions are awaited or executed before committing the Firestore batch so Redis and Firestore stay consistent; also apply the same Redis cleanup when removing bio links if they can create cached redirects.
101-117: 💤 Low valueBatch count may exceed BATCH_LIMIT before commit.
Each link with a
shortCodeadds 2 operations (link + analytics delete), but the limit check only happens after both are added. Starting at 399 operations, adding a link with analytics brings it to 401 before the check triggers.This won't fail since Firestore's actual limit is 500, but for consistency with the stated BATCH_LIMIT, check before adding the analytics delete.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/admin.js` around lines 101 - 117, The loop can exceed BATCH_LIMIT because you add the analytics delete after already incrementing batchOps for the link; modify the logic in the loop that processes linksSnapshot.docs so that you check batch capacity before adding the analytics delete: after deleting the main doc and incrementing batchOps, if linkData.shortCode then check whether adding one more operation would hit BATCH_LIMIT (e.g. batchOps >= BATCH_LIMIT - 1 or batchOps + 1 >= BATCH_LIMIT) and commit currentBatch/reset batchOps/replace currentBatch before calling db.collection(COLLECTIONS.ANALYTICS).doc(firestoreId).delete(); keep references to currentBatch, batchOps, BATCH_LIMIT, COLLECTIONS.ANALYTICS, and the toFirestoreId(...) call to locate where to change the code.src/routes/bio.js (1)
63-78: ⚖️ Poor tradeoffRace conditions in uniqueness checks.
Between checking slug availability (lines 64-70) and creating the document (line 97), another request could claim the same slug. Similarly, the one-bio-link-per-user check (lines 72-78) has the same window.
For a low-traffic application this is unlikely, but if strict uniqueness is required, consider using Firestore transactions or using the slug as the document ID to leverage Firestore's create-if-not-exists semantics.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/bio.js` around lines 63 - 78, The current uniqueness checks (existingSlug, userBioLinks) are racy because they query then later create the document; update the create flow in src/routes/bio.js to perform an atomic check-and-create – either wrap the logic in a Firestore transaction that queries COLLECTIONS.BIO_LINKS for slug and userId and creates the new doc only if both checks still pass, or switch to using the slug as the document ID and call db.collection(COLLECTIONS.BIO_LINKS).doc(slug).create(...) (or the SDK-equivalent) which will fail if the slug doc already exists; if you need to enforce one-bio-per-user as well, include that query+create inside the same transaction (use symbols existingSlug, userBioLinks, slug, userId, COLLECTIONS.BIO_LINKS to locate and change the code).src/routes/import.js (1)
24-41: ⚖️ Poor tradeoffSequential writes are inefficient — consider using batch writes.
Each link is written with a separate
await db.collection().add()call. For large imports this is slow and lacks atomicity. Firestore supports batch writes (up to 500 operations) that would improve performance and allow rollback on partial failure.Also applies to: 46-63, 67-84
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/import.js` around lines 24 - 41, The loop that calls await db.collection(COLLECTIONS.LINKS).add(...) for each link is inefficient and non-atomic; replace the per-item adds in the import route with Firestore WriteBatch usage: create a batch, for each link generateShortCode() and prepare the linkData object (including admin.firestore.FieldValue.serverTimestamp()), call batch.set(db.collection(COLLECTIONS.LINKS).doc(), linkData) or batch.create, track operations and when you reach 500 ops call batch.commit(), reset the batch, and only increment importedCount after a successful commit; ensure any remaining operations are committed at the end and surface errors from batch.commit() to maintain rollback/atomic behavior.src/utils/analytics.js (1)
131-133: ⚡ Quick winCache the dynamic import to avoid repeated resolution overhead.
When
globalThis.fetchis unavailable (Node < 18), a new dynamic import promise is created on everyfetchGeolocationcall. Consider caching the fetcher.♻️ Proposed fix
At module scope:
+let _fetch = null; +async function getFetch() { + if (!_fetch) { + _fetch = typeof globalThis.fetch === 'function' + ? globalThis.fetch + : (await import('node-fetch')).default; + } + return _fetch; +}Then in the function:
async function fetchGeolocation(clientIP) { let locationData = { country: 'Unknown', city: 'Unknown', region: 'Unknown' }; try { - const fetch = typeof globalThis.fetch === 'function' - ? globalThis.fetch - : (...args) => import('node-fetch').then(({ default: fetchFn }) => fetchFn(...args)); + const fetch = await getFetch(); const geoResponse = await fetch(`http://ip-api.com/json/${clientIP}?fields=status,country,regionName,city`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/analytics.js` around lines 131 - 133, The current fallback for fetch creates a new dynamic import on every call (used by fetchGeolocation), causing repeated resolution overhead; move the dynamic import to module scope by creating a cached promise or cached fetcher (e.g., cachedFetchPromise or cachedFetcher) that imports node-fetch once and resolves to the fetch function, then make the existing fetch variable use that cached result so fetchGeolocation and other callers reuse the same imported fetch instead of recreating the import each call.src/middleware/auth.middleware.js (1)
54-55: ⚡ Quick winMove
requireto module scope.The dynamic require inside the function is evaluated on every call. Moving it to the top of the file improves clarity and avoids repeated module resolution.
♻️ Proposed fix
At the top of the file:
const { getAuth, getDatabase, getFirebaseState, COLLECTIONS } = require('../../config/firebase.config'); +const { toFirestoreId } = require('../utils/url.utils');Then in the function:
async function requireLinkOwnership(req, res, next) { const { shortCode } = req.params; const decodedShortCode = decodeURIComponent(shortCode); const userId = req.user.uid; - const { toFirestoreId } = require('../utils/url.utils'); const firestoreId = toFirestoreId(decodedShortCode);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/middleware/auth.middleware.js` around lines 54 - 55, The dynamic require for toFirestoreId should be moved to module scope to avoid resolving the module on every call: add a top-level require for toFirestoreId (const { toFirestoreId } = require('../utils/url.utils')) at the top of src/middleware/auth.middleware.js, then remove the inline require and use the toFirestoreId identifier directly where firestoreId = toFirestoreId(decodedShortCode) is called inside the function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/routes/admin.js`:
- Around line 87-95: The DELETE /api/user/account handler calls getDatabase()
and uses db.collection(...) without checking db; add a null/undefined guard
after const db = getDatabase() in the router.delete('/api/user/account',
verifyToken, async (req, res) => { ... }) handler: if db is falsy, respond with
a 503 (or appropriate) error JSON and return to avoid calling
db.collection(COLLECTIONS.LINKS) and other Firestore ops; keep the existing
try/catch for runtime errors but ensure the early null-check prevents the
TypeError when Firestore is unavailable.
- Around line 106-108: The inline require('../utils/url.utils').toFirestoreId
inside the loop should be moved to the top with the other imports: add a single
import binding (e.g. const { toFirestoreId } = require('../utils/url.utils'))
near the file's requires and replace the inline call in the loop with
toFirestoreId(linkData.shortCode); this removes the per-iteration module lookup
and makes the usage clear (references: toFirestoreId, currentBatch.delete,
COLLECTIONS.ANALYTICS).
In `@src/routes/analytics.js`:
- Around line 27-39: The current linksData mapping reads the raw analytics
document via db.collection(COLLECTIONS.ANALYTICS).doc(firestoreId).get() which
misses shard totals; replace that direct read with a call to
getAggregatedAnalytics(firestoreId) (from src/utils/analytics.js) and return its
result in the analytics field (preserve shortCode and linkData), keeping the
same try/catch error handling as before so errors yield analytics: null; update
references to toFirestoreId and COLLECTIONS.ANALYTICS only as needed to derive
the firestoreId.
In `@src/routes/bug-report.js`:
- Line 16: The console.log call is emitting PII (name and email); remove those
from logs and only output non-PII fields (e.g., subject) or a non-identifying
token; specifically update the console.log that currently references name,
email, subject to log only subject (or a userId/token) and if you must retain a
reference to the user, log a one-way hash of email instead of the raw email;
ensure you edit the console.log statement that currently passes the name and
email variables.
In `@src/routes/import.js`:
- Line 27: Validate incoming link URLs before saving: for the branches that set
originalUrl (link.url), long_url (link.long_url), and destination
(link.destination) validate that the property exists, is non-empty and is a
well-formed URL (e.g., via a shared isValidUrl helper) and skip/record the entry
if invalid instead of persisting; update the import handler that iterates over
link items (the code that references originalUrl, link.long_url,
link.destination) to use the validator and increment a skipped counter (or
collect skipped entries) and include that info in the response so
malformed/missing URLs are not written to the DB.
- Line 16: The handler currently calls getDatabase() and uses db.collection()
without guarding for Firebase being unavailable; update the handler in
src/routes/import.js to check getFirebaseState().enabled (or that getDatabase()
!== null) immediately after calling getDatabase() and return an appropriate
503/400 response (or skip Firestore logic) when Firebase is disabled/null so
db.collection() is never invoked on null; locate references to getDatabase(),
db.collection(), and the route handler function in this file to implement the
guard and short-circuit behavior.
In `@src/routes/links.js`:
- Around line 218-221: The handler calls getDatabase() and immediately uses
db.collection(...) which will throw if Firestore is unavailable; update the code
around getDatabase(), toFirestoreId, and linkRef to check if db is
null/undefined (similar to the existing check in the GET /api/user/links
handler) and return an appropriate error response (or early exit) when db is not
available instead of calling db.collection(COLLECTIONS.LINKS).doc(...).
- Around line 327-329: The try block assumes getDatabase() always returns a
valid db; add a null/undefined check immediately after calling getDatabase()
(used with toFirestoreId(shortCode)) and handle the unavailable Firestore case
by logging the error and returning an appropriate failure response (e.g., 503 or
an error JSON) instead of proceeding — ensure the check references getDatabase()
and shortCode/toFirestoreId so the code exits early if db is falsy.
- Around line 273-275: The code calls getDatabase() and immediately uses
db.collection(...) without checking for a null/undefined db; update the try
block around getDatabase() (where getDatabase(), db, and inactiveLinksQuery are
used) to verify db is present before calling db.collection(COLLECTIONS.LINKS),
and if db is missing, log an error or return a safe response (or throw a
controlled error) so the function does not crash when Firestore is unavailable.
- Around line 249-252: The try block that calls getDatabase() in the link
handlers (where getDatabase(), toFirestoreId(shortCode), and
db.collection(COLLECTIONS.LINKS).doc(...) are used) lacks a null/undefined check
for db; add an early guard after calling getDatabase() to detect missing
Firestore (e.g., if (!db) ...) and return or throw a controlled error/HTTP
response (consistent with the deactivate handler behavior) instead of proceeding
to call db.collection. Ensure the same pattern is applied wherever getDatabase()
is used in this file to prevent crashes.
- Line 81: The health check result from checkLinkHealth(finalUrl) is computed
but never persisted; update the code that builds or saves the link document
(e.g., where linkData is constructed and persisted in the route handler) to
include the healthData (or its relevant fields) before calling the DB
save/update function, or if you intend it to be non-blocking, remove the await
call and call checkLinkHealth(finalUrl) asynchronously (fire-and-forget) and
persist when it completes; reference checkLinkHealth, healthData, finalUrl and
the linkData save/update flow so the health status is stored with the link.
In `@src/routes/profile.js`:
- Around line 55-60: The code checks userData from userDoc but later calls
db.collection(...).doc(userId).update(updateData) which will throw if the
document doesn't exist; change the update path to safely create-or-merge the
profile by using the document reference's set(updateData, { merge: true }) (or
check existence and create an empty profile before calling update). Locate
references to userDoc, userData and the update call
(db.collection(...).doc(userId).update) and replace or guard the update with a
set(..., { merge: true }) to avoid "No document to update" errors when the user
document is missing.
- Around line 25-29: The createdAt field is a Firestore sentinel
(admin.firestore.FieldValue.serverTimestamp()) so returning newProfile
immediately will contain the unresolved placeholder; after calling
db.collection(COLLECTIONS.USERS).doc(userId).set(newProfile) re-read the
just-written document via db.collection(COLLECTIONS.USERS).doc(userId).get() and
use the returned snapshot.data() (or convert its Timestamp to a JS date/ISO)
when sending res.json({ profile: ... }) instead of returning the original
newProfile.
- Around line 118-127: The route currently assumes getDatabase() returns a valid
db and on errors returns { available: true }, risking crashes and false
positives; update the handler to null-check the result of getDatabase() (use the
db variable returned by getDatabase()) and if db is null/undefined respond with
an error status (e.g., 503) and available: false, and when catching exceptions
from toFirestoreId or db.collection(COLLECTIONS.LINKS).doc(...).get() log the
full error and respond with a safe failure (status 500 or 503 and available:
false) instead of { available: true } so callers don’t get false availability;
keep references to getDatabase, toFirestoreId, COLLECTIONS.LINKS, and res.json
when implementing the changes.
- Line 92: The code calls getDatabase() and assigns to db without checking for
null; update the route handler in src/routes/profile.js to verify db is non-null
after const db = getDatabase() (or handle a falsy return) and return an
appropriate error response or throw if Firestore is unavailable; specifically
wrap subsequent uses of db (in the profile route handler) in a guard that logs
the failure and sends a 503/500 JSON error (or next(err)) so the route doesn't
dereference a null db.
- Line 40: The code calls getDatabase() and uses the resulting db without
verifying availability; add a null/undefined check after const db =
getDatabase() and handle the missing Firestore client (e.g., log an error and
return an appropriate HTTP error response or call next(err) inside the profile
route handler) so subsequent uses of db in this module (references to db) never
run when getDatabase() fails; ensure the check uses strict null/undefined
detection and provides a clear log message and an early return/error path.
- Around line 10-13: The route uses getDatabase() and then immediately calls
db.collection(COLLECTIONS.USERS).doc(userId).get() which will throw if db is
null; add the same defensive null check used in other routes (e.g., if (!db) {
/* respond with 503 or appropriate error */ }) before attempting to access db in
this handler (reference getDatabase, db, COLLECTIONS.USERS and userId) and
return an appropriate error response when Firestore is unavailable.
In `@src/routes/split-test.js`:
- Line 18: getDatabase() can return null and db.collection(...) will throw; add
a guard after calling getDatabase() in split-test.js to bail out (or return an
appropriate response) when db is null, mirroring the pattern used in import.js
and the 50-50 route. Locate the const db = getDatabase() line and check db for
falsy/null before any use (e.g., before db.collection or reads/writes), and
ensure the handler responds with the same error/redirect behavior used in other
route modules to keep behavior consistent.
- Around line 33-35: The variants mapping passed into linkRef.update is storing
v.url without validation and may save undefined/invalid URLs; update the handler
that builds splitTest (the variants.map(...) passed to linkRef.update) to
validate each variant.url (e.g., ensure presence and valid URL format) before
writing: reject the request or filter out invalid entries and return a clear
error to the caller if any variant lacks a valid url, and only call
linkRef.update({ splitTest: { variants: [...] } }) with guaranteed valid url
strings.
---
Minor comments:
In `@config/firebase.config.js`:
- Around line 46-50: The catch block in config/firebase.config.js currently
captures error but never uses it; either log the error inside the catch (e.g.,
call console.error with a descriptive message plus the error) to preserve
debugging info when Firebase Admin fails to configure, or if the error is
intentionally ignored rename the parameter to _ (catch (_)) to satisfy the
linter; update the catch around the Firebase init logic accordingly (the
existing catch(error) shown in the diff).
In `@server.js`:
- Line 102: Remove the unused import by deleting the const memoryStore =
require('./src/services/memory.service'); line in server.js since memoryStore is
not referenced by the expiry notification interval or anywhere else in this
file; ensure no other code depends on memoryStore and run tests/lint to confirm
no remaining references.
In `@src/routes/admin.js`:
- Line 3: Remove the unused destructured symbol "admin" from the require
statement so only the used exports (getDatabase, getFirebaseState, COLLECTIONS)
are imported; update the require(...) line that currently destructures {
getDatabase, getFirebaseState, COLLECTIONS, admin } to exclude admin and save
the file to clear the ESLint unused-import warning.
In `@src/routes/analytics.js`:
- Line 3: The import line currently destructures getFirebaseState but it's
unused; update the require statement to remove getFirebaseState from the
destructured symbols (leave getDatabase and COLLECTIONS) and ensure there are no
remaining references to getFirebaseState elsewhere in this module (if there are,
either remove those references or reintroduce a proper usage). This fixes the
unused-import ESLint warning for getFirebaseState.
In `@src/routes/bio.js`:
- Line 3: The import destructures getFirebaseState but it isn't used; remove
getFirebaseState from the require statement so the line reads only include
getDatabase, COLLECTIONS, and admin (i.e., update the destructuring in the
require on that line to remove getFirebaseState) and confirm there are no other
references to getFirebaseState in this file.
- Around line 198-201: Replace the hardcoded collection name in the db query
with the shared constant: change the db.collection('bioLinks') call that creates
bioLinksSnapshot to use COLLECTIONS.BIO_LINKS instead so it matches other
endpoints and centralizes the collection name; update the line that builds
bioLinksSnapshot (the db.collection(...) .where(...).limit(1).get() chain) to
reference COLLECTIONS.BIO_LINKS.
In `@src/routes/bug-report.js`:
- Line 17: The user-facing response in the bug report route currently returns
res.json({ success: true, message: 'Bug report received (logged locally)' })
while only logging to console; either make that behavior explicit in the message
or add a persistent fallback. In the POST /bug-report handler (the route handler
that calls console.log and returns res.json), either 1) change the message to
clearly state non-persistence and no guaranteed follow-up (e.g., "Bug report
received — logged locally; no persistence or guaranteed follow-up"), or 2)
implement simple persistence (e.g., append the report to a durable file via
fs.appendFile or push it to your existing queue/DB) and then update the res.json
message to reflect that the report was persisted; update references to the
handler/res.json and any console.log usage accordingly.
In `@src/routes/import.js`:
- Line 5: The import line currently destructures both generateShortCode and
toFirestoreId from '../utils/url.utils' but toFirestoreId is unused; update the
require statement to only destructure generateShortCode (remove toFirestoreId),
verify there are no remaining references to toFirestoreId in this module
(functions/handlers in src/routes/import.js) and run ESLint to confirm the
warning is resolved.
In `@src/routes/links.js`:
- Line 3: Remove the unused getFirebaseState identifier from the destructuring
require statement in src/routes/links.js; update the const declaration that
currently reads "const { getDatabase, getFirebaseState, COLLECTIONS, admin } =
require(...)" to only extract the used symbols (getDatabase, COLLECTIONS, admin)
so the linter warning is resolved.
- Around line 27-28: The catch block in the links route currently uses an unused
exception variable `e`; rename it to `_e` (or `_`) in the catch clause to
satisfy ESLint's unused-variable rule (e.g., change `catch (e)` to `catch (_e)`
in the route handler in src/routes/links.js) so the error is intentionally
discarded while leaving the existing `return res.status(400).json({ error:
'Invalid URL' });` behavior unchanged.
In `@src/routes/pages.js`:
- Line 3: The require line imports unused symbols; remove getDatabase and
COLLECTIONS from the destructuring require (the line that currently reads "const
{ getDatabase, COLLECTIONS } = require('../../config/firebase.config');") or
replace it with only the exports actually used in this module so there are no
unused imports; ensure no other references to getDatabase or COLLECTIONS remain
in the file after the change.
In `@src/routes/profile.js`:
- Line 3: Remove the unused getFirebaseState import from the destructured
require on the top of the module (the line that currently destructures
getDatabase, getFirebaseState, COLLECTIONS, admin); modify the destructuring to
only include used symbols (getDatabase, COLLECTIONS, admin) and run lint/tests
to ensure nothing else in profile.js references getFirebaseState before
committing.
In `@src/routes/split-test.js`:
- Line 34: The weight mapping for splitTest currently uses v.weight || 50 which
accepts non-numeric or negative values; update the mapping in splitTest so the
weight is coerced to a number and clamped to a valid positive range (e.g., parse
Number(v.weight), fallback to 50 when not finite, then clamp/round to a
reasonable range like 0–100) when building variants (reference the splitTest
object, variants.map and v.weight).
In `@src/routes/tracking.js`:
- Around line 247-248: The character-class regex /[.#$\/\[\]]/g escapes / and [
unnecessarily; update both replace calls (browserType.replace and
referrerSource.replace) to use a simplified class without those extra
backslashes, e.g. /[.#$[\]]/g, so you remove the unnecessary escapes while still
handling the literal ] correctly.
---
Nitpick comments:
In `@src/middleware/auth.middleware.js`:
- Around line 54-55: The dynamic require for toFirestoreId should be moved to
module scope to avoid resolving the module on every call: add a top-level
require for toFirestoreId (const { toFirestoreId } =
require('../utils/url.utils')) at the top of src/middleware/auth.middleware.js,
then remove the inline require and use the toFirestoreId identifier directly
where firestoreId = toFirestoreId(decodedShortCode) is called inside the
function.
In `@src/routes/admin.js`:
- Around line 52-77: The current code loads the entire collection via
db.collection(COLLECTIONS.LINKS).get() into linksSnapshot which can OOM/timeout
for large collections; change the sync to a paginated cursor loop (use a query
with .limit(pageSize) and .startAfter(lastDoc)) or use Firestore streaming,
iterating pages and processing each doc with the existing logic that calls
redisUtils.storeLinkInRedis and redirectCache.set (preserve synced and errors
counters and the per-doc try/catch). Ensure you update lastDoc after each page
and continue until a page returns no documents to avoid loading all docs at
once.
- Around line 86-147: The deletion flow in router.delete('/api/user/account',
verifyToken) omits removing Redis/redirect-cache entries for each deleted link;
update the link deletion loop (where linksSnapshot is iterated and
toFirestoreId(linkData.shortCode) is used) to also delete the Redis keys for
that shortCode (e.g., `link:{shortCode}` and `redirect-cache:{shortCode}`) —
obtain the Redis client from your existing cache module, prefer batching via
pipeline/multi when deleting many keys to avoid round-trips, and ensure these
Redis deletions are awaited or executed before committing the Firestore batch so
Redis and Firestore stay consistent; also apply the same Redis cleanup when
removing bio links if they can create cached redirects.
- Around line 101-117: The loop can exceed BATCH_LIMIT because you add the
analytics delete after already incrementing batchOps for the link; modify the
logic in the loop that processes linksSnapshot.docs so that you check batch
capacity before adding the analytics delete: after deleting the main doc and
incrementing batchOps, if linkData.shortCode then check whether adding one more
operation would hit BATCH_LIMIT (e.g. batchOps >= BATCH_LIMIT - 1 or batchOps +
1 >= BATCH_LIMIT) and commit currentBatch/reset batchOps/replace currentBatch
before calling db.collection(COLLECTIONS.ANALYTICS).doc(firestoreId).delete();
keep references to currentBatch, batchOps, BATCH_LIMIT, COLLECTIONS.ANALYTICS,
and the toFirestoreId(...) call to locate where to change the code.
In `@src/routes/analytics.js`:
- Around line 53-70: getDatabase() can return null which causes a TypeError when
calling db.collection(...); add an explicit null check immediately after const
db = getDatabase() and handle the Firestore-unavailable case with a clear log
and the same in-memory fallback or response used elsewhere (do not let the
TypeError fall into the generic Firestore catch). Specifically, in the route
containing toFirestoreId(shortCode), COLLECTIONS.LINKS lookup and
getAggregatedAnalytics(firestoreId), check if (!db) then call the existing
in-memory analytics path or return an appropriate response (and log "Firestore
not available" or similar) before attempting db.collection() so the error
message and control flow remain correct.
In `@src/routes/bio.js`:
- Around line 63-78: The current uniqueness checks (existingSlug, userBioLinks)
are racy because they query then later create the document; update the create
flow in src/routes/bio.js to perform an atomic check-and-create – either wrap
the logic in a Firestore transaction that queries COLLECTIONS.BIO_LINKS for slug
and userId and creates the new doc only if both checks still pass, or switch to
using the slug as the document ID and call
db.collection(COLLECTIONS.BIO_LINKS).doc(slug).create(...) (or the
SDK-equivalent) which will fail if the slug doc already exists; if you need to
enforce one-bio-per-user as well, include that query+create inside the same
transaction (use symbols existingSlug, userBioLinks, slug, userId,
COLLECTIONS.BIO_LINKS to locate and change the code).
In `@src/routes/bug-report.js`:
- Around line 9-11: Add a basic email format check after the existing presence
checks in the bug report route handler: validate the email string (variable
email) against a simple regex (RFC-lite) and if it fails return
res.status(400).json({ error: 'Invalid email format' });. Update the conditional
that currently checks !name || !email || !message to either include a separate
branch for format validation or add an if block using the regex; reference the
variables name, email, message and the Express response object res when
implementing the validation and error response.
In `@src/routes/import.js`:
- Around line 24-41: The loop that calls await
db.collection(COLLECTIONS.LINKS).add(...) for each link is inefficient and
non-atomic; replace the per-item adds in the import route with Firestore
WriteBatch usage: create a batch, for each link generateShortCode() and prepare
the linkData object (including admin.firestore.FieldValue.serverTimestamp()),
call batch.set(db.collection(COLLECTIONS.LINKS).doc(), linkData) or
batch.create, track operations and when you reach 500 ops call batch.commit(),
reset the batch, and only increment importedCount after a successful commit;
ensure any remaining operations are committed at the end and surface errors from
batch.commit() to maintain rollback/atomic behavior.
In `@src/routes/links.js`:
- Around line 31-37: Remove the redundant blockedSchemes loop: since the prior
URL protocol validation already permits only "http:" and "https:", delete the
blockedSchemes array and the for-loop that checks urlLower.startsWith(scheme)
(the variables blockedSchemes and urlLower and that conditional return). This
simplifies the route handler (links.js) and avoids unreachable checks while
preserving the existing protocol validation logic.
In `@src/utils/analytics.js`:
- Around line 131-133: The current fallback for fetch creates a new dynamic
import on every call (used by fetchGeolocation), causing repeated resolution
overhead; move the dynamic import to module scope by creating a cached promise
or cached fetcher (e.g., cachedFetchPromise or cachedFetcher) that imports
node-fetch once and resolves to the fetch function, then make the existing fetch
variable use that cached result so fetchGeolocation and other callers reuse the
same imported fetch instead of recreating the import each call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed129c53-e010-4bcf-86d6-5f5bcf348d9c
📒 Files selected for processing (18)
config/env.jsconfig/firebase.config.jsserver.jssrc/middleware/auth.middleware.jssrc/middleware/firebase.middleware.jssrc/routes/admin.jssrc/routes/analytics.jssrc/routes/bio.jssrc/routes/bug-report.jssrc/routes/import.jssrc/routes/links.jssrc/routes/pages.jssrc/routes/profile.jssrc/routes/split-test.jssrc/routes/tracking.jssrc/services/memory.service.jssrc/utils/analytics.jssrc/utils/response.js
Comment on lines
+106
to
+108
| if (linkData.shortCode) { | ||
| const firestoreId = require('../utils/url.utils').toFirestoreId(linkData.shortCode); | ||
| currentBatch.delete(db.collection(COLLECTIONS.ANALYTICS).doc(firestoreId)); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Move require to top of file.
Inline require() inside a loop is inefficient (module is cached, but the lookup happens each iteration) and unconventional. Move this import to the top of the file with other imports.
♻️ Proposed fix
At the top of the file:
const redisUtils = require('../utils/redis.utils');
const redirectCache = require('../utils/redirect-cache.utils');
+const { toFirestoreId } = require('../utils/url.utils');In the loop:
if (linkData.shortCode) {
- const firestoreId = require('../utils/url.utils').toFirestoreId(linkData.shortCode);
+ const firestoreId = toFirestoreId(linkData.shortCode);
currentBatch.delete(db.collection(COLLECTIONS.ANALYTICS).doc(firestoreId));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/admin.js` around lines 106 - 108, The inline
require('../utils/url.utils').toFirestoreId inside the loop should be moved to
the top with the other imports: add a single import binding (e.g. const {
toFirestoreId } = require('../utils/url.utils')) near the file's requires and
replace the inline call in the loop with toFirestoreId(linkData.shortCode); this
removes the per-iteration module lookup and makes the usage clear (references:
toFirestoreId, currentBatch.delete, COLLECTIONS.ANALYTICS).
Comment on lines
+118
to
+127
| const db = getDatabase(); | ||
|
|
||
| try { | ||
| const firestoreId = toFirestoreId(shortCode); | ||
| const doc = await db.collection(COLLECTIONS.LINKS).doc(firestoreId).get(); | ||
| res.json({ available: !doc.exists }); | ||
| } catch (error) { | ||
| console.error('Error checking shortcode:', error); | ||
| res.json({ available: true }); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing null check for db and unsafe error fallback.
Two issues:
- No null check for
db- will crash when Firestore unavailable - Line 126 returns
{ available: true }on error, which could cause short code collisions if users create links based on this false positive
Proposed fix
const { shortCode } = req.params;
const db = getDatabase();
+ if (!db) {
+ return res.json({ available: false, error: 'Database unavailable' });
+ }
+
try {
const firestoreId = toFirestoreId(shortCode);
const doc = await db.collection(COLLECTIONS.LINKS).doc(firestoreId).get();
res.json({ available: !doc.exists });
} catch (error) {
console.error('Error checking shortcode:', error);
- res.json({ available: true });
+ res.json({ available: false, error: 'Error checking availability' });
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/profile.js` around lines 118 - 127, The route currently assumes
getDatabase() returns a valid db and on errors returns { available: true },
risking crashes and false positives; update the handler to null-check the result
of getDatabase() (use the db variable returned by getDatabase()) and if db is
null/undefined respond with an error status (e.g., 503) and available: false,
and when catching exceptions from toFirestoreId or
db.collection(COLLECTIONS.LINKS).doc(...).get() log the full error and respond
with a safe failure (status 500 or 503 and available: false) instead of {
available: true } so callers don’t get false availability; keep references to
getDatabase, toFirestoreId, COLLECTIONS.LINKS, and res.json when implementing
the changes.
Comment on lines
+33
to
+35
| await linkRef.update({ | ||
| splitTest: { variants: variants.map(v => ({ url: v.url, weight: v.weight || 50, name: v.name || '' })) } | ||
| }); |
Contributor
There was a problem hiding this comment.
Missing validation for variant.url — variants without URLs will break redirects.
Each variant requires a url to redirect to, but the mapping v => ({ url: v.url, ... }) does not validate that v.url exists or is a valid URL. Storing undefined or invalid URLs will cause runtime failures during split-test redirects.
🛡️ Proposed fix
+ // Validate all variants have URLs
+ if (!variants.every(v => v.url && typeof v.url === 'string')) {
+ return res.status(400).json({ error: 'Each variant must have a valid URL' });
+ }
const db = getDatabase();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/split-test.js` around lines 33 - 35, The variants mapping passed
into linkRef.update is storing v.url without validation and may save
undefined/invalid URLs; update the handler that builds splitTest (the
variants.map(...) passed to linkRef.update) to validate each variant.url (e.g.,
ensure presence and valid URL format) before writing: reject the request or
filter out invalid entries and return a clear error to the caller if any variant
lacks a valid url, and only call linkRef.update({ splitTest: { variants: [...] }
}) with guaranteed valid url strings.
Group A (cleanup):
- Remove unused memoryStore import from server.js
- Remove unused getFirebaseState from bio.js, analytics.js, links.js, profile.js
- Remove unused imports from pages.js
- Rename unused catch param in firebase.config.js
- Move inline require in auth.middleware.js to module-level
- Fix hardcoded 'bioLinks' -> COLLECTIONS.BIO_LINKS in bio.js
Group B (null guards):
- Add db null guards to analytics.js, bio.js, import.js,
links.js, profile.js, split-test.js
Group C (moderate):
- Remove PII from bug-report.js console.log; add email validation
- Add URL validation on imported links in import.js
- Persist health check data in links.js
- Use {merge:true} and re-read after profile document create
Also: fix unnecessary regex escapes in tracking.js (ESLint errors)
Contributor
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 (4)
src/routes/bug-report.js (2)
27-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd type validation for
nameandmessagebefore calling.trim().Similar to the
nameormessageare not strings (e.g., numbers, objects), calling.trim()at lines 27 or 30 will throw aTypeError, resulting in a 500 error instead of a proper 400 validation response.✅ Proposed fix to add type guards
+ if (typeof name !== 'string' || typeof message !== 'string') { + return res.status(400).json({ error: 'Name and message must be strings' }); + } + const bugReport = { name: name.trim(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/bug-report.js` around lines 27 - 30, Validate that both name and message are strings before calling .trim(): in the bug report route handler check typeof name === 'string' and typeof message === 'string' (like the existing email guard), return a 400 response when either is missing or not a string, and only then call name.trim() and message.trim() when building the payload; update the validation logic around the variables name and message in src/routes/bug-report.js so non-string inputs don't reach .trim() and instead produce the proper 400 validation response.
6-6:⚠️ Potential issue | 🟠 MajorApply
bugReportLimitertoPOST /api/bug-report
bugReportLimiteris defined/exported insrc/middleware/security.middleware.jsbut is not applied anywhere in the JS codebase, leaving this endpoint unrate-limited and vulnerable to abuse/quota exhaustion.🔒 Proposed fix to apply rate limiter
const express = require('express'); const router = express.Router(); const { getDatabase, admin } = require('../../config/firebase.config'); +const { bugReportLimiter } = require('../middleware/security.middleware'); // ============ SUBMIT bug report ============ -router.post('/api/bug-report', async (req, res) => { +router.post('/api/bug-report', bugReportLimiter, async (req, res) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/bug-report.js` at line 6, Apply the existing rate limiter middleware to the bug report POST route: import or reference the bugReportLimiter middleware and add it to the router.post('/api/bug-report', ...) call so the route handler uses bugReportLimiter before the async handler; update the router.post invocation that currently only shows router.post('/api/bug-report', async (req, res) => { ... }) to include bugReportLimiter as the middleware argument (e.g., router.post('/api/bug-report', bugReportLimiter, async (req, res) => { ... })) and ensure bugReportLimiter is correctly imported from the module that exports it.src/routes/links.js (1)
269-270:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the same cache contract on reactivation.
deactivateremoves both Redis andredirectCache, butreactivateonly writes Redis and writes a different shape thanPOST /api/shorten(destinationis missing). That leaves the fast-path caches inconsistent after reactivation and can break cached redirects.💡 Proposed fix
- await linkRef.update({ isActive: true, deactivatedAt: admin.firestore.FieldValue.delete(), scheduledDeletion: admin.firestore.FieldValue.delete() }); - await redisUtils.storeLinkInRedis(shortCode, { ...linkData, isActive: true }); + const reactivatedLink = { ...linkData, isActive: true }; + await linkRef.update({ + isActive: true, + deactivatedAt: admin.firestore.FieldValue.delete(), + scheduledDeletion: admin.firestore.FieldValue.delete() + }); + await redisUtils.storeLinkInRedis(shortCode, { + destination: reactivatedLink.originalUrl, + userId: reactivatedLink.userId, + createdAt: Date.now(), + title: reactivatedLink.title || '' + }); + await redirectCache.set(shortCode, normalizeRedirectLink(reactivatedLink));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/links.js` around lines 269 - 270, When reactivating a link, restore the same cache contract used by POST /api/shorten and the redirect fast-path: after calling linkRef.update(...) and before/when calling redisUtils.storeLinkInRedis(shortCode, ...), include the full shape (ensure the payload contains destination plus any other fields POST /api/shorten writes) and also repopulate the redirect cache the same way deactivate removed it (i.e., write the same entry to redirectCache as POST /api/shorten would). Update the code paths around linkRef.update and redisUtils.storeLinkInRedis to use linkData.destination (and other missing fields) and to call the same redirectCache set routine so caches remain consistent after reactivation.src/routes/import.js (1)
29-43:⚠️ Potential issue | 🟠 MajorStore imported links under
toFirestoreId(shortCode)(use deterministic Firestore doc IDs)
src/routes/links.jsreads/writes links viadb.collection(COLLECTIONS.LINKS).doc(toFirestoreId(shortCode))(wheretoFirestoreIdreplaces/with_).src/routes/import.jscurrently persists imported links with...collection(...).add(linkData), which creates random document IDs—so imported links won’t be found by the normal link-management handlers, and imported custom slugs aren’t checked for collisions in the deterministic key space.💡 Proposed fix
- await db.collection(COLLECTIONS.LINKS).add(linkData); + const firestoreId = toFirestoreId(shortCode); + await db.collection(COLLECTIONS.LINKS).doc(firestoreId).create(linkData);Handle
already-existsby skipping (or reporting conflict) instead of leaving unreachable duplicates/overwriting.Also applies to: 54-68, 78-92
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/import.js` around lines 29 - 43, The import routine currently uses db.collection(COLLECTIONS.LINKS).add(linkData) which creates random IDs and breaks lookups in handlers that use db.collection(COLLECTIONS.LINKS).doc(toFirestoreId(shortCode)); change the persistence to use db.collection(COLLECTIONS.LINKS).doc(toFirestoreId(shortCode)).set(linkData) so the document ID is deterministic (toFirestoreId replaces '/' with '_'), and when calling generateShortCode ensure you compute the Firestore ID via toFirestoreId(shortCode) before writing; also handle Firestore conflict errors (already-exists) by skipping or reporting the conflict instead of overwriting (or swallow if intentional) so imported custom slugs are checked for collisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/routes/bug-report.js`:
- Around line 13-17: The validation currently calls email.trim() without
ensuring email is a string, causing a TypeError for non-string inputs; update
the check to first verify typeof email === 'string' (or coerce safely) and only
then call email.trim() and test against emailRegex, e.g. return a 400 when
typeof email !== 'string' or when emailRegex.test(email.trim()) fails; reference
the existing emailRegex and the use of email.trim() in the bug-report route to
locate where to add the type guard and error response.
In `@src/routes/import.js`:
- Around line 26-27: The import routine currently only skips unparseable URLs;
update each validation (the spots checking new URL(link.url) — around the
link.url check at the top and the similar checks at the other two locations) to
also enforce HTTP(S)-only destinations by inspecting the parsed URL's protocol
(e.g., const u = new URL(link.url); if (u.protocol !== 'http:' && u.protocol !==
'https:') continue/skip). Ensure you apply this same protocol check where new
URL(link.url) is used so non-HTTP(S) targets are rejected during import the same
way /api/shorten does.
---
Outside diff comments:
In `@src/routes/bug-report.js`:
- Around line 27-30: Validate that both name and message are strings before
calling .trim(): in the bug report route handler check typeof name === 'string'
and typeof message === 'string' (like the existing email guard), return a 400
response when either is missing or not a string, and only then call name.trim()
and message.trim() when building the payload; update the validation logic around
the variables name and message in src/routes/bug-report.js so non-string inputs
don't reach .trim() and instead produce the proper 400 validation response.
- Line 6: Apply the existing rate limiter middleware to the bug report POST
route: import or reference the bugReportLimiter middleware and add it to the
router.post('/api/bug-report', ...) call so the route handler uses
bugReportLimiter before the async handler; update the router.post invocation
that currently only shows router.post('/api/bug-report', async (req, res) => {
... }) to include bugReportLimiter as the middleware argument (e.g.,
router.post('/api/bug-report', bugReportLimiter, async (req, res) => { ... }))
and ensure bugReportLimiter is correctly imported from the module that exports
it.
In `@src/routes/import.js`:
- Around line 29-43: The import routine currently uses
db.collection(COLLECTIONS.LINKS).add(linkData) which creates random IDs and
breaks lookups in handlers that use
db.collection(COLLECTIONS.LINKS).doc(toFirestoreId(shortCode)); change the
persistence to use
db.collection(COLLECTIONS.LINKS).doc(toFirestoreId(shortCode)).set(linkData) so
the document ID is deterministic (toFirestoreId replaces '/' with '_'), and when
calling generateShortCode ensure you compute the Firestore ID via
toFirestoreId(shortCode) before writing; also handle Firestore conflict errors
(already-exists) by skipping or reporting the conflict instead of overwriting
(or swallow if intentional) so imported custom slugs are checked for collisions.
In `@src/routes/links.js`:
- Around line 269-270: When reactivating a link, restore the same cache contract
used by POST /api/shorten and the redirect fast-path: after calling
linkRef.update(...) and before/when calling
redisUtils.storeLinkInRedis(shortCode, ...), include the full shape (ensure the
payload contains destination plus any other fields POST /api/shorten writes) and
also repopulate the redirect cache the same way deactivate removed it (i.e.,
write the same entry to redirectCache as POST /api/shorten would). Update the
code paths around linkRef.update and redisUtils.storeLinkInRedis to use
linkData.destination (and other missing fields) and to call the same
redirectCache set routine so caches remain consistent after reactivation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b38ddac6-8ca9-452d-b8cb-8a321a1b94d2
📒 Files selected for processing (12)
config/firebase.config.jsserver.jssrc/middleware/auth.middleware.jssrc/routes/analytics.jssrc/routes/bio.jssrc/routes/bug-report.jssrc/routes/import.jssrc/routes/links.jssrc/routes/pages.jssrc/routes/profile.jssrc/routes/split-test.jssrc/routes/tracking.js
💤 Files with no reviewable changes (1)
- src/routes/pages.js
🚧 Files skipped from review as they are similar to previous changes (7)
- src/middleware/auth.middleware.js
- src/routes/analytics.js
- src/routes/profile.js
- src/routes/tracking.js
- src/routes/split-test.js
- server.js
- src/routes/bio.js
Comment on lines
+26
to
+27
| if (!link.url) continue; | ||
| try { new URL(link.url); } catch (_) { continue; } |
Contributor
There was a problem hiding this comment.
Apply the same HTTP(S)-only validation here as /api/shorten.
These branches only reject unparseable URLs. src/routes/links.js explicitly blocks non-HTTP(S) targets before creating a redirectable link, so imports can currently bypass that protection and persist unsafe destinations.
Also applies to: 51-52, 75-76
🧰 Tools
🪛 GitHub Actions: CI / lint
[warning] 27-27: ESLint 'no-unused-vars': '_' is defined but never used.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/import.js` around lines 26 - 27, The import routine currently only
skips unparseable URLs; update each validation (the spots checking new
URL(link.url) — around the link.url check at the top and the similar checks at
the other two locations) to also enforce HTTP(S)-only destinations by inspecting
the parsed URL's protocol (e.g., const u = new URL(link.url); if (u.protocol !==
'http:' && u.protocol !== 'https:') continue/skip). Ensure you apply this same
protocol check where new URL(link.url) is used so non-HTTP(S) targets are
rejected during import the same way /api/shorten does.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/routes/bug-report.js (3)
30-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd type validation before calling
.trim()onname.Following the same pattern as the
nameis a string before calling.trim(). If a client sends a non-string value like{"name": 123}, line 30 will throwTypeError: name.trim is not a function, resulting in a 500 error instead of a proper 400 validation error.✅ Proposed fix to add type guard
if (!name || !email || !message) { return res.status(400).json({ error: 'Name, email, and message are required' }); } + if (typeof name !== 'string') { + return res.status(400).json({ error: 'Name must be a string' }); + } + // Basic email validation if (typeof email !== 'string') {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/bug-report.js` at line 30, The code calls name.trim() without checking type, which can throw if name is not a string; update the input validation to mirror the existing email check by verifying typeof name === "string" (and that it's non-empty after trimming) before calling name.trim(), and if the check fails return the same 400 validation response path used for email so non-string inputs like { "name": 123 } produce a 400 error instead of a 500; look for the handler that parses name and email (the name.trim() usage) and apply the type guard there.
33-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd type validation before calling
.trim()onmessage.Following the same pattern as the
messageis a string before calling.trim(). If a client sends a non-string value like{"message": ["array"]}, line 33 will throwTypeError: message.trim is not a function, resulting in a 500 error instead of a proper 400 validation error.✅ Proposed fix to add type guard
if (typeof email !== 'string') { return res.status(400).json({ error: 'Email must be a string' }); } + if (typeof name !== 'string') { + return res.status(400).json({ error: 'Name must be a string' }); + } + if (typeof message !== 'string') { + return res.status(400).json({ error: 'Message must be a string' }); + } + const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/bug-report.js` at line 33, The code calls message.trim() without ensuring message is a string; add a type guard similar to the existing email validation: check typeof message === 'string' (or use a validator helper used elsewhere) before calling message.trim(), and return a 400 validation error if it's not a string so malformed bodies (e.g., {"message": ["array"]}) don't throw a TypeError; only assign or pass message.trim() after the check (update the spot referencing message.trim() and follow the same response pattern used for the email validation).
6-11:⚠️ Potential issue | 🔴 CriticalCritical: Frontend
/api/bug-reportpayload doesn’t match backend validation (UI always gets 400).
src/routes/bug-report.jsdestructures{ name, email, subject, message, type }and immediately returns400 "Name, email, and message are required"whennameormessageis missing (lines 7-11).
Butpublic/js/app.jssends{ title, description, steps, email, userId, userEmail }(lines 49-61), with nonameand nomessage—so every frontend submission is rejected.Align the request schema (e.g., map
title -> subject,description -> message, and sendnameor update the backend to accept the frontend field names). Also decide how/ifstepsshould be persisted (backend currently ignores it).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/bug-report.js` around lines 6 - 11, The backend validation in router.post('/api/bug-report') currently expects { name, email, subject, message, type } and rejects when name or message are missing, while the frontend sends { title, description, steps, email, userId, userEmail }; update the handler to accept and map the frontend fields (map title -> subject, description -> message, userEmail/userId -> name or populate name from userEmail/userId) or make these fields optional; also persist or accept steps (e.g., add a steps variable from req.body and include it in DB/save logic) so submissions from public/js/app.js are not always rejected.
🧹 Nitpick comments (1)
src/routes/bug-report.js (1)
36-36: ⚖️ Poor tradeoffConsider privacy implications of storing IP addresses.
Storing
req.ipin Firestore creates a PII retention concern under GDPR/CCPA. Additionally,req.ipmay not provide the correct client IP when the application runs behind proxies or load balancers (which typically require parsingX-Forwarded-Forheaders). Consider whether IP address collection is necessary for bug reports, and if retained, document the privacy policy and retention period.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/bug-report.js` at line 36, The code currently writes the raw client IP via req.ip (the line setting ip: req.ip) which raises PII/retention and proxy-correctness concerns; either stop storing the IP, or replace it with a privacy-preserving alternative (e.g., store a hashed or truncated form or only the /24 subnet) and, if you must capture a client IP, derive it robustly from X-Forwarded-For before falling back to req.ip. Update the bug-report creation logic that sets ip: req.ip to use the chosen approach and add/update documentation/retention metadata for stored IPs in your privacy policy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/routes/bug-report.js`:
- Line 30: The code calls name.trim() without checking type, which can throw if
name is not a string; update the input validation to mirror the existing email
check by verifying typeof name === "string" (and that it's non-empty after
trimming) before calling name.trim(), and if the check fails return the same 400
validation response path used for email so non-string inputs like { "name": 123
} produce a 400 error instead of a 500; look for the handler that parses name
and email (the name.trim() usage) and apply the type guard there.
- Line 33: The code calls message.trim() without ensuring message is a string;
add a type guard similar to the existing email validation: check typeof message
=== 'string' (or use a validator helper used elsewhere) before calling
message.trim(), and return a 400 validation error if it's not a string so
malformed bodies (e.g., {"message": ["array"]}) don't throw a TypeError; only
assign or pass message.trim() after the check (update the spot referencing
message.trim() and follow the same response pattern used for the email
validation).
- Around line 6-11: The backend validation in router.post('/api/bug-report')
currently expects { name, email, subject, message, type } and rejects when name
or message are missing, while the frontend sends { title, description, steps,
email, userId, userEmail }; update the handler to accept and map the frontend
fields (map title -> subject, description -> message, userEmail/userId -> name
or populate name from userEmail/userId) or make these fields optional; also
persist or accept steps (e.g., add a steps variable from req.body and include it
in DB/save logic) so submissions from public/js/app.js are not always rejected.
---
Nitpick comments:
In `@src/routes/bug-report.js`:
- Line 36: The code currently writes the raw client IP via req.ip (the line
setting ip: req.ip) which raises PII/retention and proxy-correctness concerns;
either stop storing the IP, or replace it with a privacy-preserving alternative
(e.g., store a hashed or truncated form or only the /24 subnet) and, if you must
capture a client IP, derive it robustly from X-Forwarded-For before falling back
to req.ip. Update the bug-report creation logic that sets ip: req.ip to use the
chosen approach and add/update documentation/retention metadata for stored IPs
in your privacy policy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 737ce003-144f-4f40-8462-866f49405965
📒 Files selected for processing (3)
src/routes/admin.jssrc/routes/analytics.jssrc/routes/bug-report.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/routes/admin.js
- src/routes/analytics.js
rishab11250
added a commit
to rishab11250/piik.me
that referenced
this pull request
Jun 10, 2026
Merge upstream/main into refactor/split-server-js. Kept refactored modular route structure (HEAD) while adding upstream's validateEnv() guard for required environment variables. Changes: - Add validateEnv() to check required vars + SESSION_SECRET length - Keep all modular route imports and tracking router - Socket.IO + server start auto-merged correctly - resolve content conflict in server.js
rishab11250
force-pushed
the
refactor/split-server-js
branch
from
d548c51 to
132061b
Compare
Merge upstream/main into refactor/split-server-js. Kept refactored modular route structure (HEAD) while adding upstream's validateEnv() guard for required environment variables. Changes: - Add validateEnv() to check required vars + SESSION_SECRET length - Keep all modular route imports and tracking router - Socket.IO + server start auto-merged correctly - resolve content conflict in server.js
rishab11250
force-pushed
the
refactor/split-server-js
branch
from
132061b to
fb345aa
Compare
This was referenced Jun 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Split the monolithic
server.js(~1975 lines) into 11 focused route modules, 4 middleware files, a config layer, and utility services. The newserver.jsis 120 lines — a 94% reduction.New structure
config/env.js,firebase.config.jssrc/routes/index.js,links.js,analytics.js,bio.js,profile.js,tracking.js,split-test.js,bug-report.js,import.js,admin.js,pages.jssrc/middleware/auth.middleware.js,error.middleware.js,security.middleware.js,firebase.middleware.jssrc/services/memory.service.js,splitTest.service.jssrc/utils/analytics.js,response.js,url.utils.js,redis.utils.js,redirect-cache.utils.js,checkLinkHealth.jsKey changes
initializeFirebase(),getDatabase(),getAuth(),getFirebaseState(),admin,COLLECTIONS— initialized once, shared across all modules.PORT,NODE_ENV,ALLOWED_ORIGIN,isServerless).requireLinkOwnershipandrequireBioOwnershipmiddleware extracted from inline checks.verifyTokennow imports fromconfig/firebase.config.js.getLink,setLink,deleteLink,getAllLinks,hasLink,getAnalytics,setAnalytics,deleteAnalytics,getOrCreateAnalytics,incrementAnalytics.createTrackingRouter({ io })injects Socket.IO for real-time analytics. Contains impression/share tracking + catch-all redirect routes./→landing.html,/login//dashboard→index.html, etc.).Verification
npm run devstarts successfully. All 11 endpoint categories tested and returning expected status codes:Related Issue
Fixes #217
Type of Change
Checklist
src/structure, ownership-verified endpoints)Notes
createTrackingRouter({ io })) rather than being a module-level singleton — enables clean testing and serverless compatibility.server.js./:shortCode,/:username/:slug)..github/ISSUES/anddocs/GSSOC-ANALYSIS-REPORT.mdfiles in the working tree are unrelated to this PR.Summary by CodeRabbit
New Features
Improvements