fix: code quality and security refactor — audit-driven fixes across monorepo#61
Merged
fix: code quality and security refactor — audit-driven fixes across monorepo#61
Conversation
…llback gazetteer - Fix ambiguous match test: use 'LANG' (present in gazetteer) instead of 'ANAHAW' (not present) - Fix exact match test: use 'ANAHAW' (present) instead of 'LANIT' (not present) - This aligns test expectations with actual gazetteer data Fixes failing tests identified in monorepo code quality audit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add 'include: ['src/**/*.test.ts']' to vitest.config.ts - Prevents running tests from both src/ and lib/ directories - Fixes issue where test count was inflated (190 vs expected) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lures
- Step2WhoWhere.tsx: type 2 bare catch {} blocks (_err: unknown) for localStorage/sessionStorage
- useOnlineStatus.ts: type bare catch {} in network probe
- draft-store.ts: type bare catch {} in isBlobReadable
- Prevents silent failures while respecting localStorage unavailability in private browsing
Part of code quality audit remediation.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- shared-sms-parser/src/inbound.ts: type bare catch {} in gazetteer require fallback
- shared-validators/src/msisdn.ts: type bare catch {} in node:crypto require fallback
- Improves error visibility while maintaining graceful degradation
Part of code quality audit remediation.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- fcm-send.ts: capture and append FCM retry error to warnings; keep outer catch bare (intentional retry) - semaphore.ts: include original parse error in thrown SmsProviderRetryableError - sms-inbound.ts: capture and log MSISDN normalization error - on-media-finalize.ts: capture and log corrupt-image processing error - sms-inbound-processor.ts: capture and log MSISDN decryption error - inbox-reconciliation-sweep.ts: restore bare catch with explicit comment (transaction contention is intentional skip) Improves debugging visibility while maintaining intentional retry/skip behavior. Part of code quality audit remediation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…dling - Add 8 tests for requireAuth, bantayogErrorToHttps, and error code mapping - Covers auth_required, permission-denied, and validation error paths - Tests normalize-to-client behavior for BantayogError→HttpsError conversion - Ensures auth boundaries are correctly enforced and surfaced Part of code quality audit remediation - adds missing test coverage for auth-critical code. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add refactor-audit-2026-04-23.md with comprehensive audit findings (P0-P3 issues) - Update progress.md with completed code quality refactor work - Update learnings.md with key learnings from the audit - Documents 13 bare catch blocks fixed, 2 test failures resolved, and new test coverage added Provides baseline for ongoing code quality improvements and prevents future regression. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…otency - Regenerated .d.ts.map and .js.map files from source changes - Build artifacts from catch block typing fixes in src/msisdn.ts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Reviewer's GuideMonorepo-wide refactor that fixes SMS parser tests, tightens error handling by typing and logging previously silent catches, adds targeted auth-boundary tests in Cloud Functions, and documents the audit and refactor progress. Sequence diagram for smsInboundWebhookCore MSISDN hashing and error loggingsequenceDiagram
participant Client
participant CloudFunctions as smsInboundWebhookCore
participant MsisdnValidator as normalizeMsisdn
participant Hasher as hashMsisdn
participant Crypto as crypto_sha256
participant Logger as log
Client->>CloudFunctions: HTTP POST /sms-inbound (rawFrom)
activate CloudFunctions
Note over CloudFunctions: Try to normalize MSISDN and compute salted hash
CloudFunctions->>MsisdnValidator: normalizeMsisdn(rawFrom)
MsisdnValidator-->>CloudFunctions: normalizedMsisdn
CloudFunctions->>Hasher: hashMsisdn(normalizedMsisdn, salt)
Hasher-->>CloudFunctions: msisdnHash
deactivate CloudFunctions
CloudFunctions-->>Client: 200 OK (processed)
%% Error path introduced in this PR
Note over Client,CloudFunctions: Invalid MSISDN path
Client->>CloudFunctions: HTTP POST /sms-inbound (invalid rawFrom)
activate CloudFunctions
CloudFunctions->>MsisdnValidator: normalizeMsisdn(rawFrom)
MsisdnValidator-->>CloudFunctions: throws Error
CloudFunctions-->>CloudFunctions: catch (err: unknown)
CloudFunctions->>Crypto: sha256(rawFrom)
Crypto-->>CloudFunctions: msisdnHashFallback
CloudFunctions->>Logger: log({ severity: WARNING, code: msisdn.invalid, message: Invalid MSISDN received, data: { rawFromMasked, error: String(err) } })
CloudFunctions-->>Client: 200 OK (processed with fallback hash)
deactivate CloudFunctions
Sequence diagram for smsInboundProcessor MSISDN decryption failure loggingsequenceDiagram
participant Firestore as Firestore
participant Trigger as smsInboundProcessor
participant Crypto as decryptMsisdn
participant Logger as log
participant SmsProvider as sendSmsReply
Firestore->>Trigger: onDocumentCreated(inboundSms)
activate Trigger
Trigger-->>Trigger: read senderMsisdnEnc and msgId
Trigger->>Crypto: decryptMsisdn(senderMsisdnEnc)
alt decryption succeeds
Crypto-->>Trigger: recipientMsisdn
Trigger->>SmsProvider: sendSmsReply(recipientMsisdn, template)
SmsProvider-->>Trigger: ack
Trigger-->>Firestore: update doc status (reply_sent)
else decryption fails
Crypto-->>Trigger: throws Error err
Trigger-->>Trigger: catch (err: unknown)
Trigger->>Logger: log({ severity: WARNING, code: decrypt.failed, message: MSISDN decryption failed for msgId, data: { error: String(err) } })
Trigger-->>Firestore: update doc or skip reply
end
deactivate Trigger
Class diagram for shared validators and SMS parser error-handling changesclassDiagram
class SharedValidatorsMsisdn {
- _nodeCrypto: createHash(CreateHashFn) | null
+ getNodeCrypto(): createHash(CreateHashFn) | null
}
class SharedValidatorsIdempotency {
+ canonicalPayloadHash(payload: any): Promise~string~
- canonicalize(value: any): any
}
class SharedSmsParserInbound {
+ getBarangayGazetteer(): BarangayEntry[]
}
SharedValidatorsMsisdn <.. SharedValidatorsIdempotency : uses crypto.subtle
SharedSmsParserInbound ..> SharedValidatorsMsisdn : uses hashing utilities
%% Details of crypto fallback and error handling
class NodeCryptoFallback {
+ tryRequireNodeCrypto(): createHash(CreateHashFn) | null
- handleRequireError(_err: unknown): void
}
SharedValidatorsMsisdn o-- NodeCryptoFallback : composition
class WebCryptoHasher {
+ canonicalPayloadHash(payload: any): Promise~string~
- getSubtleCrypto(): SubtleCrypto
}
SharedValidatorsIdempotency o-- WebCryptoHasher : composition
class GazetteerLoader {
+ getBarangayGazetteer(): BarangayEntry[]
- loadSharedDataModule(): any
- handleLoadError(_err: unknown): void
}
SharedSmsParserInbound o-- GazetteerLoader : composition
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces bare Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
canonicalPayloadHashyou removed thesubtlefeature check and now assumeglobalThis.crypto.subtlealways exists; consider restoring a guard and throwing a clear error or using a fallback to avoid runtime failures in environments without Web Crypto. - In
sendFcmToResponder, appendingString(err)to thewarningsarray may introduce noisy or sensitive data into a field that might be surfaced to clients or logs; consider logging the full error only in server-side logging and keeping the warning codes themselves stable and predictable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `canonicalPayloadHash` you removed the `subtle` feature check and now assume `globalThis.crypto.subtle` always exists; consider restoring a guard and throwing a clear error or using a fallback to avoid runtime failures in environments without Web Crypto.
- In `sendFcmToResponder`, appending `String(err)` to the `warnings` array may introduce noisy or sensitive data into a field that might be surfaced to clients or logs; consider logging the full error only in server-side logging and keeping the warning codes themselves stable and predictable.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Add Web Crypto API availability check in canonicalPayloadHash with clear error message - Add test coverage for missing Web Crypto API scenario using vi.stubGlobal - Change sendFcmToResponder to log FCM errors server-side only, keep warnings as stable codes - Prevents noisy/sensitive data in client-facing warnings while maintaining debug visibility Addresses review feedback on PR #61. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![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: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/refactor-audit-2026-04-23.md`:
- Around line 124-128: Update the guidance to exempt intentional Firebase
emulator mock casts by noting that "as any" in test files under the
functions/src/__tests__ tree (e.g., functions/src/__tests__/rules/*.test.ts,
functions/src/__tests__/callables/close-report.test.ts,
functions/src/__tests__/acceptance/phase-4a-acceptance.test.ts) are allowed for
emulator compatibility and should not be counted as tech-debt; keep the existing
action to replace other `any` usages with proper Firestore types or `unknown`,
but add a clear exception sentence and pattern-based exclusion for the
emulator-related test files.
- Around line 29-33: The refactor-audit doc incorrectly lists bare `catch {}`
blocks that were already fixed; update the inventory to reflect current code by
removing or correcting the entries that reference the now-converted bare catches
and any stale line ranges, and refresh the “Action”/“Effort” items accordingly;
specifically, remove the stale bare-catch mentions and adjust the suggested
extractions to match the current components/hooks (GpsButton,
MunicipalitySelector, BarangaySelector, ContactFields and hooks useGpsLocation,
useMunicipalityBarangays), ensuring the document only calls out real remaining
issues and accurate locations.
- Around line 4-18: The P0 section in docs/refactor-audit-2026-04-23.md is out
of date: it still lists two failing tests in shared-sms-parser even though this
PR includes the fixes; update the "Health Check" and "## 🔴 P0 — Fix Before
Anything Else" content to reflect that packages/shared-sms-parser tests now pass
(remove or mark resolved the two failing items about inbound.test.ts and the two
specific failures) and adjust the status indicators (Lint/Typecheck/Tests) so
the audit baseline matches the current branch state.
In `@functions/src/__tests__/callables/https-error.test.ts`:
- Around line 11-17: The test currently iterates
Object.keys(BANTAYOG_TO_HTTPS_CODE) which only checks the map's existing keys
and can miss unmapped enum entries; change the loop to iterate the actual
BantayogErrorCode enum values (e.g., use Object.values(BantayogErrorCode) cast
to BantayogErrorCode[]) and for each enum value assert
BANTAYOG_TO_HTTPS_CODE[value] is defined and typeof === 'string' so every enum
member is validated against the BANTAYOG_TO_HTTPS_CODE mapping.
In `@functions/src/http/sms-inbound.ts`:
- Around line 96-103: The WARNING log in the catch block for msisdn parsing
currently includes String(err), which may leak PII; update the catch handling
around msisdnHash and the log(...) invocation to avoid logging raw error
text—replace String(err) with a sanitized token such as error.name and/or
error.code (e.g., err instanceof Error ? err.name : 'UnknownError') or a fixed
safe message, or redact digits from the error string before logging; ensure you
modify the catch block that computes msisdnHash and calls log({ severity:
'WARNING', code: 'msisdn.invalid', ... }) so only non-PII error identifiers are
included.
In `@functions/src/services/fcm-send.ts`:
- Around line 69-72: The catch block in fcm-send.ts currently pushes String(err)
into the warnings array, mixing raw transport text with machine-readable codes;
change the catch to only push stable warning codes (e.g., keep
'fcm_network_error' or add another pre-defined code) and remove the String(err)
push so the returned warnings remain machine-readable. Instead of returning the
raw error, log/record the full error to your internal logger or monitoring (call
logger.error(err, { context: 'fcm-send' }) or send to Sentry) from inside the
same catch block so you preserve diagnostics without exposing internal details
to clients. Ensure the variable names referenced are warnings and the catch in
the sendFcm/fcm-send handler are updated accordingly and the API response only
contains the sanitized codes.
In `@functions/src/services/sms-providers/semaphore.ts`:
- Around line 62-66: The catch block that currently throws
SmsProviderRetryableError for unparseable responses needs to treat 4xx responses
as non-retryable: if res.status is in the 400–499 range, throw the non-retryable
error type (e.g., SmsProviderPermanentError or the project's equivalent) with a
clear message including res.status and the parse error; only construct
SmsProviderRetryableError for server-side/5xx errors (and network-like
failures). Update the logic around SmsProviderRetryableError creation in the
catch to branch on res.status (>=500 = retryable/provider_error, 400–499 =
non-retryable/invalid_request) and include the original error details in the
message.
In `@packages/shared-sms-parser/src/inbound.ts`:
- Around line 45-48: The current catch in inbound.ts swallows every error when
trying to load '@bantayog/shared-data', returning FALLBACK_BARANGAYS even for
real runtime/data errors; update the catch around the dynamic import/load so it
only suppresses expected module-not-found errors (e.g., check (err as
NodeJS.ErrnoException).code === 'MODULE_NOT_FOUND' or inspect the error message
for the package name or ERR_MODULE_NOT_FOUND) and rethrow or propagate any other
errors so real failures surface; keep the fallback behavior for the specific
missing-module case and reference FALLBACK_BARANGAYS and the
'@bantayog/shared-data' load location when making the change.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 761d1303-db44-44fc-b508-825e679d28af
⛔ Files ignored due to path filters (5)
packages/shared-sms-parser/lib/inbound.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/idempotency.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/idempotency.js.mapis excluded by!**/*.mappackages/shared-validators/lib/msisdn.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/msisdn.js.mapis excluded by!**/*.map
📒 Files selected for processing (18)
apps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsxapps/citizen-pwa/src/hooks/useOnlineStatus.tsapps/citizen-pwa/src/services/draft-store.tsdocs/learnings.mddocs/progress.mddocs/refactor-audit-2026-04-23.mdfunctions/src/__tests__/callables/https-error.test.tsfunctions/src/firestore/sms-inbound-processor.tsfunctions/src/http/sms-inbound.tsfunctions/src/services/fcm-send.tsfunctions/src/services/sms-providers/semaphore.tsfunctions/src/triggers/on-media-finalize.tspackages/shared-sms-parser/src/__tests__/inbound.test.tspackages/shared-sms-parser/src/inbound.tspackages/shared-validators/lib/idempotency.jspackages/shared-validators/lib/msisdn.jspackages/shared-validators/src/msisdn.tspackages/shared-validators/vitest.config.ts
Comment on lines
+62
to
66
| } catch (err: unknown) { | ||
| throw new SmsProviderRetryableError( | ||
| `semaphore ${res.status.toString()}: unparseable response`, | ||
| `semaphore ${res.status.toString()}: unparseable response (${String(err)})`, | ||
| res.ok || res.status >= 500 ? 'provider_error' : 'network', | ||
| ) |
Contributor
There was a problem hiding this comment.
Non-retryable 4xx parse failures are being retried.
At Line 65, a non-JSON 4xx response can be classified as retryable and deferred, even when it’s likely a permanent request/config issue.
🔧 Proposed fix
let data: SemaphoreResponse = {}
try {
data = (await res.json()) as SemaphoreResponse
} catch (err: unknown) {
+ if (!res.ok && res.status < 500 && res.status !== 429) {
+ return { accepted: false, reason: 'other' as const, latencyMs: 0 }
+ }
throw new SmsProviderRetryableError(
`semaphore ${res.status.toString()}: unparseable response (${String(err)})`,
- res.ok || res.status >= 500 ? 'provider_error' : 'network',
+ res.status === 429 ? 'rate_limited' : 'provider_error',
)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/services/sms-providers/semaphore.ts` around lines 62 - 66, The
catch block that currently throws SmsProviderRetryableError for unparseable
responses needs to treat 4xx responses as non-retryable: if res.status is in the
400–499 range, throw the non-retryable error type (e.g.,
SmsProviderPermanentError or the project's equivalent) with a clear message
including res.status and the parse error; only construct
SmsProviderRetryableError for server-side/5xx errors (and network-like
failures). Update the logic around SmsProviderRetryableError creation in the
catch to branch on res.status (>=500 = retryable/provider_error, 400–499 =
non-retryable/invalid_request) and include the original error details in the
message.
… 4xx retry, MODULE_NOT_FOUND - docs/refactor-audit-2026-04-23.md: Update P0 status (tests now pass), mark fixed bare catches, add Firebase emulator exception for test any casts - https-error.test.ts: Iterate enum values instead of map keys to catch unmapped entries - sms-inbound.ts: Replace String(err) with error.name to avoid logging PII in MSISDN validation errors - semaphore.ts: Treat 4xx responses as non-retryable (return accepted:false), only retry 5xx/429 - inbound.ts: Only catch MODULE_NOT_FOUND errors, rethrow real runtime failures Addresses all CodeRabbit review feedback on PR #61. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/shared-validators/src/idempotency.ts`:
- Around line 21-29: The current guard only checks typeof subtle === 'object'
which lets null through; update the check around the Web Crypto usage in
idempotency.ts to validate that globalThis.crypto.subtle is present and usable
before calling subtle.digest: explicitly verify !subtle || typeof subtle.digest
!== 'function' (or equivalent truthy check) and throw the original descriptive
Error if the guard fails; ensure this check sits immediately before using
canonicalize, JSON.stringify, TextEncoder, and subtle.digest so you never call
subtle.digest on null/undefined.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 29db3ba9-57ad-478c-a4ae-9e8d350423dc
⛔ Files ignored due to path filters (3)
packages/shared-validators/lib/idempotency.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/idempotency.js.mapis excluded by!**/*.mappackages/shared-validators/lib/idempotency.test.js.mapis excluded by!**/*.map
📒 Files selected for processing (5)
functions/src/services/fcm-send.tspackages/shared-validators/lib/idempotency.jspackages/shared-validators/lib/idempotency.test.jspackages/shared-validators/src/idempotency.test.tspackages/shared-validators/src/idempotency.ts
- Change guard from typeof subtle !== 'object' to !subtle || typeof subtle.digest !== 'function' - Catches null case (typeof null === 'object' is JS quirk) where subtle is explicitly null - Validates digest method exists before calling it - Add test coverage for crypto.subtle = null scenario Prevents calling subtle.digest on null, which would fail with cryptic error instead of our intended descriptive error message about Web Crypto API unavailability. Addresses CodeRabbit review feedback on PR #61. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
docs/refactor-audit-2026-04-23.md (3)
138-140:⚠️ Potential issue | 🟡 MinorExecution order includes resolved tasks.
Line 138 and Line 140 still schedule already-completed work (
shared-sms-parserP0 fix andinbound.tsbare-catch fix), which makes the roadmap misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/refactor-audit-2026-04-23.md` around lines 138 - 140, The roadmap lists resolved tasks as pending — remove the completed entries for "shared-sms-parser" (P0 fix) and the "inbound.ts" bare-catch fix from the execution order so the schedule only shows outstanding work; instead mark those items as completed or archived and ensure entries referencing Step2WhoWhere.tsx, shared-sms-parser, and inbound.ts are updated to reflect status (e.g., "Done" or linked to the completing PR) and regenerate the execution order so it no longer includes resolved tasks.
79-99:⚠️ Potential issue | 🟡 MinorBare-catch inventory/counts are outdated.
Line 95 still lists
functions/src/services/sms-providers/semaphore.tsas a bare-catch site, and Line 154 still reports 13 bare catches. These values should be recalculated from current branch state.Also applies to: 154-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/refactor-audit-2026-04-23.md` around lines 79 - 99, The audit's "Inconsistent error handling (6+ catch patterns)" section contains stale data: it still lists functions/src/services/sms-providers/semaphore.ts as a bare-catch site and the total bare-catch count (13) is outdated — recalculate the current catch-pattern counts across the branch and update the summary counts and the file list in that section (the heading "Inconsistent error handling (6+ catch patterns)" and its bullet list). Ensure you run the same detection script/regex used originally, verify intentional exceptions (e.g., MODULE_NOT_FOUND or retry logic) remain annotated, remove entries already fixed, and replace the old numbers and file entries with the new, accurate values.
30-37:⚠️ Potential issue | 🟡 Minor
inbound.tsdebt entry is stale (bare catch no longer exists).Line 36 and Line 92 still claim a bare
catch {}inpackages/shared-sms-parser/src/inbound.ts, but the file now usescatch (err: unknown)with selective fallback/rethrow behavior.Also applies to: 92-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/refactor-audit-2026-04-23.md` around lines 30 - 37, The docs entry for packages/shared-sms-parser/src/inbound.ts incorrectly reports a bare `catch {}`; update the `inbound.ts` debt note to reflect that the code now uses `catch (err: unknown)` with selective fallback/rethrow behavior (remove the stale "bare catch" claim and the duplicate_comment marker). Locate the `inbound.ts` entry in the audit doc and replace the bare-catch assertion with a brief description of the current catch handling (mentioning `catch (err: unknown)` and its selective fallback/rethrow semantics) and remove any duplicate references.packages/shared-sms-parser/src/inbound.ts (1)
45-55:⚠️ Potential issue | 🟠 MajorNarrow fallback to missing
@bantayog/shared-dataonly.Line 47-Line 53 currently suppress any
MODULE_NOT_FOUNDraised inside thetry, not just the package load failure for@bantayog/shared-data. That can silently hide real runtime breakages and downgrade parser quality.🔧 Proposed fix
} catch (err: unknown) { // Only suppress MODULE_NOT_FOUND errors; rethrow real failures const isModuleNotFound = typeof err === 'object' && err !== null && 'code' in err && (err as { code?: string }).code === 'MODULE_NOT_FOUND' - if (isModuleNotFound) { + const message = err instanceof Error ? err.message : '' + const isSharedDataLoadFailure = message.includes('@bantayog/shared-data') + if (isModuleNotFound && isSharedDataLoadFailure) { return FALLBACK_BARANGAYS } throw err }As per coding guidelines "Defensive Programming: Assume external input is malicious/broken. Validate at the boundary. Never swallow errors with an empty catch block."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-sms-parser/src/inbound.ts` around lines 45 - 55, The catch currently treats any MODULE_NOT_FOUND inside the try as a harmless missing package and returns FALLBACK_BARANGAYS; instead, only swallow the error when the missing module is specifically '@bantayog/shared-data'. Change the predicate around isModuleNotFound to also check the error message (or error.path/name) contains the literal '@bantayog/shared-data' (while preserving the existing code check for err.code === 'MODULE_NOT_FOUND'); if both conditions match return FALLBACK_BARANGAYS, otherwise rethrow the error so real runtime failures in functions called inside the try are not silently suppressed. Use the same error variable and keep the throw err behavior for non-matching cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/shared-validators/lib/idempotency.js`:
- Around line 18-21: The runtime check for the Web Crypto Subtle API using
"const subtle = globalThis.crypto?.subtle;" doesn't handle null (typeof null ===
"object"), so update the guard in idempotency.js to explicitly check for
null/undefined and presence of the digest method before assuming subtle is
usable; e.g., validate "subtle" is truthy and has a "digest" function (check
subtle && typeof subtle.digest === 'function') and log/throw the original
clearer error path if missing so subsequent use of subtle.digest won't hit
"Cannot read property 'digest' of null".
---
Duplicate comments:
In `@docs/refactor-audit-2026-04-23.md`:
- Around line 138-140: The roadmap lists resolved tasks as pending — remove the
completed entries for "shared-sms-parser" (P0 fix) and the "inbound.ts"
bare-catch fix from the execution order so the schedule only shows outstanding
work; instead mark those items as completed or archived and ensure entries
referencing Step2WhoWhere.tsx, shared-sms-parser, and inbound.ts are updated to
reflect status (e.g., "Done" or linked to the completing PR) and regenerate the
execution order so it no longer includes resolved tasks.
- Around line 79-99: The audit's "Inconsistent error handling (6+ catch
patterns)" section contains stale data: it still lists
functions/src/services/sms-providers/semaphore.ts as a bare-catch site and the
total bare-catch count (13) is outdated — recalculate the current catch-pattern
counts across the branch and update the summary counts and the file list in that
section (the heading "Inconsistent error handling (6+ catch patterns)" and its
bullet list). Ensure you run the same detection script/regex used originally,
verify intentional exceptions (e.g., MODULE_NOT_FOUND or retry logic) remain
annotated, remove entries already fixed, and replace the old numbers and file
entries with the new, accurate values.
- Around line 30-37: The docs entry for
packages/shared-sms-parser/src/inbound.ts incorrectly reports a bare `catch {}`;
update the `inbound.ts` debt note to reflect that the code now uses `catch (err:
unknown)` with selective fallback/rethrow behavior (remove the stale "bare
catch" claim and the duplicate_comment marker). Locate the `inbound.ts` entry in
the audit doc and replace the bare-catch assertion with a brief description of
the current catch handling (mentioning `catch (err: unknown)` and its selective
fallback/rethrow semantics) and remove any duplicate references.
In `@packages/shared-sms-parser/src/inbound.ts`:
- Around line 45-55: The catch currently treats any MODULE_NOT_FOUND inside the
try as a harmless missing package and returns FALLBACK_BARANGAYS; instead, only
swallow the error when the missing module is specifically
'@bantayog/shared-data'. Change the predicate around isModuleNotFound to also
check the error message (or error.path/name) contains the literal
'@bantayog/shared-data' (while preserving the existing code check for err.code
=== 'MODULE_NOT_FOUND'); if both conditions match return FALLBACK_BARANGAYS,
otherwise rethrow the error so real runtime failures in functions called inside
the try are not silently suppressed. Use the same error variable and keep the
throw err behavior for non-matching cases.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 25695840-0097-47d8-ab69-17c31f1f510e
⛔ Files ignored due to path filters (3)
packages/shared-sms-parser/lib/inbound.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/idempotency.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/idempotency.js.mapis excluded by!**/*.map
📒 Files selected for processing (6)
docs/refactor-audit-2026-04-23.mdfunctions/src/__tests__/callables/https-error.test.tsfunctions/src/http/sms-inbound.tsfunctions/src/services/sms-providers/semaphore.tspackages/shared-sms-parser/src/inbound.tspackages/shared-validators/lib/idempotency.js
…, Web Crypto runtime tests - idempotency: add WeakSet-based circular reference detection in canonicalize() - idempotency: guard subtle.digest runtime failures with try/catch - idempotency.test: add test for broken subtle.digest implementation - msisdn: enforce minimum 16-character salt in hashMsisdn - msisdn.test: add salt validation tests; update existing fixtures to valid length
…yption
- Add hex regex check (/^[0-9a-fA-F]{64}$/) to ensure 64-char key
- Add decoded length check (key.length !== 32) for AES-256-GCM
- Prevents silent weak encryption from malformed env var
…e interval - Step2WhoWhere: distinguish QuotaExceededError from SecurityError in localStorage catches - useOnlineStatus: reduce PROBE_INTERVAL_MS from 30s to 10s for disaster responsiveness - useOnlineStatus: reduce PROBE_TIMEOUT_MS from 5s to 3s (fail fast) - useOnlineStatus: replace AbortSignal.timeout with AbortController for broader env support
…/shared-data only - Only swallow MODULE_NOT_FOUND when error message contains '@bantayog/shared-data' - Rethrow all other runtime failures instead of silently suppressing them
…d resolved tasks
- Recalculate catch-pattern counts: bare catch {} now 0 in production source
- Remove completed shared-sms-parser P0 fix and inbound.ts catch fix from execution order
- Mark semaphore.ts and inbound.ts as already typed (no longer bare catch)
- Update stats: bare-catch blocks 13 → 0 in production source
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/citizen-pwa/src/hooks/useOnlineStatus.ts (1)
43-44:⚠️ Potential issue | 🟡 MinorUpdate stale effect comment to match actual scheduling.
Line 43 says “every 30s” and mentions
setTimeout, but the hook now probes every 10s viasetIntervalafter an immediate call.Suggested fix
- // Probe on mount and every 30s — schedule immediately via setTimeout to avoid - // calling setState synchronously within the effect body (React lint rule) + // Probe immediately on mount, then every 10s via setInterval.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/citizen-pwa/src/hooks/useOnlineStatus.ts` around lines 43 - 44, The comment above the probing logic in useOnlineStatus is outdated (mentions “every 30s” and setTimeout) — update it to accurately describe the current behavior: it probes immediately once and then schedules recurring probes every 10s using setInterval; mention the immediate call and that setInterval is used to avoid calling setState synchronously within the effect body so readers understand the scheduling approach used in the useOnlineStatus hook.
♻️ Duplicate comments (2)
docs/refactor-audit-2026-04-23.md (2)
86-90:⚠️ Potential issue | 🟡 MinorResolve contradiction in bare-catch reporting.
This section says bare
catch {}is0in production source, but also lists two production files as remaining bare-catch locations. Please make the count and file list consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/refactor-audit-2026-04-23.md` around lines 86 - 90, Update the summary and the file list so they agree: either change the count "0 in production source" to reflect the two production files listed (functions/src/services/fcm-send.ts and functions/src/triggers/inbox-reconciliation-sweep.ts) or remove those two files from the "remaining" list if they are intentionally excluded; also clarify why the two listed occurrences are not counted as production (e.g., mark them as exceptions with a short parenthetical like "(intentional retry logic)" for fcm-send and "(intentional transaction contention skip)" for inbox-reconciliation-sweep) and make the header count match the adjusted list and explanations.
24-24:⚠️ Potential issue | 🟡 MinorCorrect the catch signature text to match the implementation.
The note says
catch (_err: unknown), but the currentStep2WhoWhere.tsximplementation usescatch (err: unknown). Please align wording to avoid audit drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/refactor-audit-2026-04-23.md` at line 24, Update the documentation line to match the implementation's catch signature: replace the note that says `catch (_err: unknown)` with `catch (err: unknown)` so it reflects the actual code in Step2WhoWhere.tsx where the catch uses `catch (err: unknown)`; ensure the wording exactly matches that symbol usage to avoid audit drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsx`:
- Around line 380-390: The catch block in Step2WhoWhere.tsx currently swallows
all errors except quota and security cases; update it to log unexpected storage
errors instead of silently discarding them by adding a console.error (or app
logger) for errors that are not DOMException quota/security cases—keep the
existing DOMException checks (err instanceof DOMException and err.name ===
'QuotaExceededError' || err.code === 22) and the intentional silent handling for
SecurityError, but for any other err value log a clear message (e.g.,
"[Step2WhoWhere] unexpected storage error") along with the error object; apply
the same change to the other catch block around the 431-440 region.
In `@apps/citizen-pwa/src/hooks/useOnlineStatus.ts`:
- Around line 26-39: The probe timeout (timeoutId) is only cleared on the
success path, leaving the timer running on errors; update the probe logic inside
useOnlineStatus (the fetch to PROBE_URL using controller.signal and
PROBE_TIMEOUT_MS) so that clearTimeout(timeoutId) is always executed (e.g., move
it into a finally block or call it in the catch before setting
setProbeOnline(false)), ensuring the AbortController (controller) and timeout
are cleaned up regardless of success or failure.
In `@packages/shared-validators/src/idempotency.test.ts`:
- Around line 63-98: Add a regression test that exercises the WeakSet-based
cycle detection by passing a self-referential object or array into
canonicalPayloadHash so the canonicalize circular-reference branch runs; for
example create const obj: any = {}; obj.self = obj; (or a self-referential
array) and then assert await expect(canonicalPayloadHash(obj)).rejects.toThrow()
(or rejects/toThrow matching your error) inside the same test style as the other
Web Crypto guards, cleaning up globals if used; this ensures the canonicalize
cycle-detection path is covered and locked in.
In `@packages/shared-validators/src/idempotency.ts`:
- Around line 31-39: The catch block around subtle.digest in idempotency.ts
currently swallows the original error; update the try/catch in the function that
computes the SHA-256 digest so the catch captures the thrown error (e.g., "err")
and rethrows or wraps it including the original error message/cause in the new
Error passed from that block, preserving provider/runtime details instead of
discarding them; reference the existing subtle.digest call and the surrounding
error thrown ('Web Crypto API (globalThis.crypto.subtle.digest) failed...') to
include the underlying error information in that message.
In `@packages/shared-validators/src/msisdn.test.ts`:
- Around line 67-78: Add a runtime non-string salt test in
packages/shared-validators/src/msisdn.test.ts to ensure hashMsisdn enforces its
boundary: call hashMsisdn('+639171234567', null), hashMsisdn('+639171234567',
undefined) and hashMsisdn('+639171234567', 12345) (or at least one
representative non-string) and expect the call to throw (matching the existing
error expectation such as 'at least 16 characters' or a TypeError) so the
function's guard against non-string salts is locked down; update the test block
alongside the existing empty/short salt cases referencing the hashMsisdn symbol.
In `@packages/shared-validators/src/msisdn.ts`:
- Around line 50-54: The current validation in hashMsisdn reads salt.length even
when salt isn't a string which can throw; update the check to avoid
dereferencing non-strings by first determining if typeof salt === 'string' and
only then reading length, and build the Error message using a safe value (e.g.
the actual string length when salt is a string or a descriptive token like
"invalid" / the typeof value otherwise). Ensure the condition still enforces
"salt must be a string of at least 16 characters" and reference the salt
parameter and the hashMsisdn validation block when making the change.
---
Outside diff comments:
In `@apps/citizen-pwa/src/hooks/useOnlineStatus.ts`:
- Around line 43-44: The comment above the probing logic in useOnlineStatus is
outdated (mentions “every 30s” and setTimeout) — update it to accurately
describe the current behavior: it probes immediately once and then schedules
recurring probes every 10s using setInterval; mention the immediate call and
that setInterval is used to avoid calling setState synchronously within the
effect body so readers understand the scheduling approach used in the
useOnlineStatus hook.
---
Duplicate comments:
In `@docs/refactor-audit-2026-04-23.md`:
- Around line 86-90: Update the summary and the file list so they agree: either
change the count "0 in production source" to reflect the two production files
listed (functions/src/services/fcm-send.ts and
functions/src/triggers/inbox-reconciliation-sweep.ts) or remove those two files
from the "remaining" list if they are intentionally excluded; also clarify why
the two listed occurrences are not counted as production (e.g., mark them as
exceptions with a short parenthetical like "(intentional retry logic)" for
fcm-send and "(intentional transaction contention skip)" for
inbox-reconciliation-sweep) and make the header count match the adjusted list
and explanations.
- Line 24: Update the documentation line to match the implementation's catch
signature: replace the note that says `catch (_err: unknown)` with `catch (err:
unknown)` so it reflects the actual code in Step2WhoWhere.tsx where the catch
uses `catch (err: unknown)`; ensure the wording exactly matches that symbol
usage to avoid audit drift.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c4eb9088-9ae7-40ff-8df2-d04d1aea12b8
⛔ Files ignored due to path filters (6)
packages/shared-validators/lib/idempotency.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/idempotency.js.mapis excluded by!**/*.mappackages/shared-validators/lib/idempotency.test.js.mapis excluded by!**/*.mappackages/shared-validators/lib/msisdn.d.ts.mapis excluded by!**/*.mappackages/shared-validators/lib/msisdn.js.mapis excluded by!**/*.mappackages/shared-validators/lib/msisdn.test.js.mapis excluded by!**/*.map
📒 Files selected for processing (13)
apps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsxapps/citizen-pwa/src/hooks/useOnlineStatus.tsdocs/refactor-audit-2026-04-23.mdfunctions/src/firestore/sms-inbound-processor.tspackages/shared-sms-parser/src/inbound.tspackages/shared-validators/lib/idempotency.jspackages/shared-validators/lib/idempotency.test.jspackages/shared-validators/lib/msisdn.jspackages/shared-validators/lib/msisdn.test.jspackages/shared-validators/src/idempotency.test.tspackages/shared-validators/src/idempotency.tspackages/shared-validators/src/msisdn.test.tspackages/shared-validators/src/msisdn.ts
Add else-if guards to both localStorage and sessionStorage catch blocks to log unexpected errors while preserving intentional silence for SecurityError (private mode). QuotaExceededError still logs warnings. Addresses CodeRabbit review: prevent silent failures for non-expected storage errors. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tatus Move clearTimeout(timeoutId) from success path to finally block to ensure AbortController timeout is always cleaned up, even when fetch fails. Also update comment to reflect actual behavior: probe every 10s via setInterval (not 30s via setTimeout). Addresses CodeRabbit review: prevent resource leaks on error paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Update subtle.digest catch block to capture and append the original error message to the thrown error, preserving provider/runtime diagnostic details. Add regression test for circular reference detection to ensure the WeakSet-based cycle detection path is exercised and locked in. Addresses CodeRabbit review: don't discard debugging context. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Split hashMsisdn validation to check typeof salt before reading .length property, preventing confusing TypeError when null/undefined is passed. Add test for non-string salt to verify type guard throws appropriate error instead of failing on property access. Addresses CodeRabbit review: avoid dereferencing non-strings. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
docs/refactor-audit-2026-04-23.md (1)
86-90:⚠️ Potential issue | 🟠 MajorReconcile contradictory
catch {}totals in this same document.Line 86 reports 2 in production source, while Line 154 reports 0 in production source. This breaks audit reliability and can mis-prioritize work.
🧭 Suggested doc correction
-- - `catch {}` — **2** in production source (intentional exceptions documented below) +- - `catch {}` — **2 intentional** occurrences in production source (documented below) ... -- - **Bare `catch {}` blocks:** 0 in production source (2 in scripts) +- **Bare `catch {}` blocks:** 2 intentional in production source (2 in scripts)Also applies to: 154-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/refactor-audit-2026-04-23.md` around lines 86 - 90, Update the contradictory counts for bare "catch {}" in docs/refactor-audit-2026-04-23.md so the summary on line 86 matches the later summary around line 154: verify the actual production occurrences (files listed: functions/src/services/fcm-send.ts, functions/src/triggers/inbox-reconciliation-sweep.ts) and either change "2 in production source" to the correct number or update the later summary to reflect "2 in production source"; also ensure the "Files with bare `catch {}`" list and the parenthetical notes for those files remain accurate and consistent with the corrected total.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsx`:
- Around line 380-391: Extract the duplicated DOMException branching into a
small helper (e.g., isQuotaExceededError(err: unknown) and isSecurityError(err:
unknown) or a single classifyStorageError returning
{quota:boolean,security:boolean}) and replace the duplicate try/catch branches
in Step2WhoWhere's storage read/write handlers with calls to that helper;
implement the helper to check err instanceof DOMException and the deprecated
code check (err.name === 'QuotaExceededError' || err.code === 22) for quota and
err.name === 'SecurityError' for security, then keep the existing logging
behavior in the catch blocks but delegate decision logic to the new helper so
both the read and write catch sites use the same classification logic.
In `@apps/citizen-pwa/src/hooks/useOnlineStatus.ts`:
- Around line 4-5: The probe timing values in useOnlineStatus.ts
(PROBE_TIMEOUT_MS and PROBE_INTERVAL_MS) are too short and may cause
online/offline flapping that triggers retries in useSubmissionMachine; increase
PROBE_TIMEOUT_MS (e.g., to 5000 ms) and PROBE_INTERVAL_MS (e.g., to 30000 ms) or
make them configurable/backoff-aware so transient mobile network hiccups don't
rapidly flip isOnline and cause retry thrashing. Ensure updates are applied
where PROBE_TIMEOUT_MS and PROBE_INTERVAL_MS are referenced so the submission
machine sees the calmer probe cadence.
In `@docs/refactor-audit-2026-04-23.md`:
- Around line 19-24: The documentation incorrectly states "707-line component"
for Step2WhoWhere.tsx; update the wording to say the file is ~707 lines while
the Step2WhoWhere component body is much smaller and the file contains large
constants and helpers. Edit the entry referencing
apps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsx and change
"707-line component" to something like "≈707-line file (component body is much
smaller; large constants/helpers inflate file size)" and ensure the item
references the component name Step2WhoWhere to keep clarity.
- Around line 52-53: The section title "Entire apps/packages have zero tests" is
inaccurate relative to the table that shows apps like `apps/responder-app`
having 1 test; update the heading text to match the table values (e.g., "Entire
apps/packages have few or no tests" or "Apps/packages with zero or few tests")
and ensure any related summary lines reference `apps/responder-app` correctly so
the title and table are consistent.
---
Duplicate comments:
In `@docs/refactor-audit-2026-04-23.md`:
- Around line 86-90: Update the contradictory counts for bare "catch {}" in
docs/refactor-audit-2026-04-23.md so the summary on line 86 matches the later
summary around line 154: verify the actual production occurrences (files listed:
functions/src/services/fcm-send.ts,
functions/src/triggers/inbox-reconciliation-sweep.ts) and either change "2 in
production source" to the correct number or update the later summary to reflect
"2 in production source"; also ensure the "Files with bare `catch {}`" list and
the parenthetical notes for those files remain accurate and consistent with the
corrected total.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 959c5e81-6e8f-4b10-b3d1-5f7c0dfffa01
📒 Files selected for processing (7)
apps/citizen-pwa/src/components/SubmitReportForm/Step2WhoWhere.tsxapps/citizen-pwa/src/hooks/useOnlineStatus.tsdocs/refactor-audit-2026-04-23.mdpackages/shared-validators/src/idempotency.test.tspackages/shared-validators/src/idempotency.tspackages/shared-validators/src/msisdn.test.tspackages/shared-validators/src/msisdn.ts
- Extract DOMException classification helpers in Step2WhoWhere.tsx
(isQuotaExceededError, isSecurityError) to deduplicate read/write
catch-block logic.
- Increase useOnlineStatus probe timings: timeout 3s→5s, interval
10s→30s to reduce mobile network flapping. Update stale comment.
- Fix contradictory catch{} counts in refactor-audit doc (2 in
production source, matching actual code).
- Correct '707-line god component' and 'zero tests' wording in
refactor-audit to match table values.
- Rebuild shared-validators lib/ artifacts to sync with source.
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
2 participants
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
Comprehensive code quality refactor addressing critical issues identified in the 2026-04-23 monorepo audit. Fixed failing tests, eliminated silent error swallowing, added missing test coverage for auth boundaries, and established baseline documentation for ongoing quality improvements.
Files Changed: 21 files across 8 workspaces (citizen-pwa, functions, shared-sms-parser, shared-validators, docs)
Changes
🔴 P0 — Fixed Failing Tests
shared-sms-parser: Fixed 2 failing tests by aligning expectations with actual gazetteer data (LANG, ANAHAW)shared-validators: Addedinclude: ['src/**/*.test.ts']to prevent duplicate test execution (was showing 190 tests instead of expected)🟠 P1 — Error Handling & Safety
catch {}blocks for localStorage/sessionStorage private-mode failurescatch {}in sms-parser (gazetteer fallback) and validators (crypto fallback)fcm-send.ts: Capture FCM retry errors while keeping outer catch bare (intentional retry)semaphore.ts: Include original parse error in thrownSmsProviderRetryableErrorsms-inbound.ts: Capture and log MSISDN normalization errorson-media-finalize.ts: Capture and log corrupt-image processing errorssms-inbound-processor.ts: Capture and log MSISDN decryption errors🟢 P3 — Test Coverage & Documentation
functions/src/__tests__/callables/https-error.test.ts(8 tests)requireAuth,bantayogErrorToHttps, and error code mappingdocs/refactor-audit-2026-04-23.md(comprehensive audit baseline)docs/progress.mdwith completed refactor workdocs/learnings.mdwith key learningsTest Plan
✅ Verification Complete:
npx turbo run lint— PASS (25/25)npx turbo run typecheck— PASS (25/25)pnpm --filter @bantayog/shared-sms-parser test— PASS (13/13, including 2 previously failing)pnpm --filter @bantayog/shared-validators test— PASS (190/190, no duplicates)pnpm --filter @bantayog/functions test src/__tests__/callables/https-error.test.ts— PASS (8/8)Impact
Related
docs/refactor-audit-2026-04-23.md🤖 Generated with Claude Code
Summary by Sourcery
Refine error handling and observability across the monorepo while fixing parser tests and capturing audit context for future quality work.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests
Documentation