Conversation
Reviewer's GuideThe PR migrates Wildfires to the shared ESLint and Nx dependency configuration, fixes resulting lint and accessibility issues, and refactors survey submission retries so they are awaited with reliable loading-state cleanup. Sequence diagram for reliable Wildfires survey submission retriessequenceDiagram
participant Hook as useSurveySubmission
participant API as submitResults API
participant Storage as localStorage
Hook->>Hook: attemptSubmission(survey, surveyID)
Hook->>API: fetch POST /api/submitResults
alt submission succeeds
API-->>Hook: successful response
Hook->>Storage: setItem(LOCAL_STORAGE_KEY, newStoredData)
Hook-->>Hook: setStatus(success)
else submission fails and retries remain
API-->>Hook: error response
Hook->>Hook: setTimeout(retryDelay)
Hook->>Hook: submitWithRetry(retries + 1)
else maximum retries reached
API-->>Hook: error response
Hook-->>Hook: setStatus(error)
end
Hook-->>Hook: isSubmitting.current = false
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
🚀 Expo continuous deployment is ready for betterangels!
iOS Simulator Build: Simulator Build Link |
|
🔍 [storybook-react] Preview available at: https://storybook.dev.betterangels.la/branches/DEV-2478-RUN-ESLINT-WITH-WILDFIRE-APP Last updated: 2026-09-10T23:59:08.875Z |
|
🔍 [shelter-web] Preview available at: https://shelter.dev.betterangels.la/branches/DEV-2478-RUN-ESLINT-WITH-WILDFIRE-APP Last updated: 2026-09-10T23:59:08.886Z |
|
🔍 [betterangels-admin] Preview available at: https://admin.dev.betterangels.la/branches/DEV-2478-RUN-ESLINT-WITH-WILDFIRE-APP Last updated: 2026-09-10T23:59:08.893Z |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
nparmar1
changed the title
tglaz
left a comment
tglaz
left a comment
There was a problem hiding this comment.
looks good - but few minor comments.
While you're doing this, would you mind updating the apps/wildfires/vite.config.mts to add a fix for font loading?
This issue already already existed, but font files get a 403, and just needs a small update to what we do in the vite.config.mts files for other apps:
--- a/apps/wildfires/vite.config.mts
+++ b/apps/wildfires/vite.config.mts
@@ -14,6 +14,7 @@ import {
const SERVER_PORT = 8200;
const SERVER_PORT_PREVIEW = 8201;
+const WORKSPACE_ROOT = path.resolve(__dirname, '../..');
export default defineConfig(({ mode }) => {
const isDev = mode === 'development';
@@ -30,6 +31,7 @@ export default defineConfig(({ mode }) => {
server: {
port: SERVER_PORT,
host: 'localhost',
+ fs: { allow: [WORKSPACE_ROOT] },
},
| First Name<span className="text-[#FF0000]">*</span> | ||
| </label> | ||
| <input | ||
| id="firstName" |
There was a problem hiding this comment.
this isn't blocking, but can you check if it'd work just as well without adding the id attribute?
the HTML id attribute needs to be unique, and firstName is kind of generic - probably ok in this app, but not best practice.
you may be able to avoid adding htmlFor + id attributes by wrapping the input with the label so you'd have something like below.
if this renders as expected then I'd change it, otherwise your implementation is ok. I wouldn't spend too much time on this app.
<label className="mb-1 font-bold">
Last Name<span className="text-[#FF0000]">*</span>
<input
style={styles.input}
onChange={handleChange}
type="text"
name="lastName"
value={formData.lastName}
required={true}
placeholder="Last Name"
/>
</label>
| } | ||
|
|
||
| function getAllRules(forms: TSurveyForm[]): TConditionRule[] { | ||
| function _getAllRules(forms: TSurveyForm[]): TConditionRule[] { |
There was a problem hiding this comment.
we should probably avoid this pattern for unused functions. I'd prefer just deleting it if unused.
|
[not blocking] |
|
Hi, @tglaz, I made the following updates addressing your feedback:
Please have a look when you have the chance. |
| } | ||
|
|
||
| function getTags(answers: any[]): string[] { | ||
| function getTags(answers: TSurveyResults['answers']): string[] { |
There was a problem hiding this comment.
isn't TSurveyResults['answers'] just TAnswer?
| setExpand(wasExpandedBeforePrintRef.current); | ||
| } | ||
| }, [isPrinting]); // ✅ Removed expand from dependencies to prevent race conditions | ||
| }, [isPrinting]); |
There was a problem hiding this comment.
optionally can look below (line 39) and incorporate the tailwindcss(suggestCanonicalClasses) suggestions.
The class `min-w-[50px]` can be written as `min-w-12.5`
| const buttonRect = buttonRef.current.getBoundingClientRect(); | ||
| const dropdownWidth = dropdownRef.current.offsetWidth; | ||
| if (!isOpen || !buttonRef.current || !dropdownRef.current) { | ||
| return; |
| useEffect(() => { | ||
| onChange && onChange(answers); | ||
| }, [answers]); | ||
| }, [answers, onChange]); |
There was a problem hiding this comment.
I want to highlight this change as this adding dependencies into this array is not always desired - and sometimes can cause a big issues as could trigger re-renders or recalculations at wrong time, or just way too often. Please review the comments below and decide on best approach:
This is a common issue to look out for with the useEffect deps.
Adding onChange to the deps changes the effect's semantics
1. Why it was deliberately keyed to answers only
This effect is the "answers changed" notification: it should fire when the data changes, and answers (a useState array in SurveyProvider) gets a new array identity exactly when an answer is added/updated. onChange is just the callback that delivers that data — it's not a trigger. The previous entry `onChange` was a constant (template literal with no interpolation), so the deps were effectively [answers], which kept the effect from re-running merely because the parent re-rendered with a new function identity.
2. What adding it does
Every caller passes an inline, un-memoized callback — FiresSurvey.tsx defines onChange in the component body — so its identity changes on every parent render. With [answers, onChange] the effect now means "run on every parent render and whenever answers change", so onChange(answers) fires repeatedly with an unchanged payload.
That's invisible today only because the current handler is a no-op. Once it does real work this becomes:
- duplicated side effects (analytics, network, logging), and
- a possible infinite loop: if the handler writes state (e.g.
storeSurveyResults(...)), the parent re-renders → newonChangeidentity → effect runs again → writes again…
Worth noting the previous constant form isn't lint-clean under our current react-hooks rules either (it reports "missing dependency" and "complex expression"), so replacing it was understandable — but this replacement trades a lint warning for a behavior change.
3. Options
- Keep
[answers]and suppress the rule on that line — smallest possible diff, and semantically valid (the effect re-runs from the latest render, so the currentonChangeis used whenever it fires; the only difference is a changed callback isn't called for data that already exists). Caveats: it may take two rules to silence here (react-hooks/exhaustive-depsand@eslint-react/exhaustive-deps), the suppression also hides future genuinely-missing deps on that line, and the intent only lives in a comment. Acceptable fallback — but since we're on React 19.2,useEffectEventgives the same behavior without opting out of the check, so should consider it. Otherwise should include a comment explaining why it's omitted so it's not added in the future:useEffect(() => { // onChange is intentionally not a trigger: this should fire only when // `answers` change, and the latest callback is read from the current render. // eslint-disable-next-line react-hooks/exhaustive-deps, @eslint-react/exhaustive-deps }, [answers]);
- Keep
[answers]and read the latest callback viauseEffectEvent(React 19.2 is already pinned in the repo; this pattern lints clean here):(the older-React equivalent is the "latest ref" pattern)const emitChange = useEffectEvent((results: TAnswer[]) => { onChange?.(results); }); useEffect(() => { emitChange(answers); // Effect Events are intentionally left out of deps }, [answers]);
- Keep
onChangein the deps, but stabilize it at the call sites: wrap the handlers inuseCallback(whatexhaustive-depssuggests) so the identity only changes when it genuinely should. - Keep as-is intentionally: acceptable only if re-firing on every render is harmless for callers (idempotent callback that never sets parent state). If that's the intent, please add a short comment saying so, otherwise the next reader will "fix" it back.
| setShow(wasExpandedBeforePrintRef.current); | ||
| } | ||
| }, [isPrinting]); // ✅ Removed show from dependencies to prevent race conditions | ||
| }, [isPrinting]); |
There was a problem hiding this comment.
Non-blocking: the print expand/collapse effect in ResourceCallout — fine to merge, worth a follow-up refactor
Not blocking for this PR, because it works today for a reason that isn't obvious from the code:
- Nothing in the app currently calls
setPrinting— the results page just mounts a print copy of the tree withinitialPrinting={true}and a screen copy with the defaultfalse. SoisPrintingis effectively static per instance, the effect runs once per mount, and both branches collapse to "set the initial expanded state". No user-visible bug. - The
[isPrinting]deps are complete; noexhaustive-depsconcern.
Why it's still worth tracking as a follow-up:
- The state updater writes
wasExpandedBeforePrintRef.current— updaters must stay pure (React double-invokes them in StrictMode and can rebase/discard them under concurrent rendering). It's idempotent today, so nothing breaks, but it's fragile if that block ever grows. - The
elsebranch unconditionally resetsshowto the ref's initialfalseon mount. It only no-ops becauseuseState(false)coincidentally matches — change the default (or initialize the ref differently) and the callout silently collapses on mount. - The whole save/restore half is dead code while
isPrintingcannot transition. - If dynamic printing is ever wired up (calling
setPrintingaroundwindow.print()), the forced-open becomes a one-shot rather than an invariant: a toggle during printing hides the content mid-print, and that toggle is then silently overwritten when printing ends. The control also stays clickable with no visible effect during that window.
Suggested follow-up (should also cover BestPracticesCard.tsx, which has the same pattern — ideally extract a small hook so it lives in one place):
const [show, setShow] = useState(false);
const { isPrinting } = usePrint();
const isOpen = isPrinting || show; // forced open while capturing for print
const handleToggle = useCallback(() => {
setShow((prev) => !prev);
}, []);Then use isOpen for aria-expanded, the chevron rotation, and the content's block/hidden accordingly, and delete the ref + effect. This keeps the callout open for the entire print capture, removes the impure updater and the mount-time reset, and behaves the same whether isPrinting stays static or later becomes dynamic. (If toggling during printing shouldn't be possible at all, also disable the toggle while isPrinting.)
Unrelated drive-by: the hardcoded id="resource-callout-content" collides across every callout instance (there are two per resource, print + screen trees), so aria-controls resolves to the wrong node — useId() fixes that.
|
|
||
| const categoryResources: TCategoryResources[] = Object.entries(grouped).map( | ||
| ([slug, entries]) => ({ | ||
| ([, entries]) => ({ |
There was a problem hiding this comment.
this works, but preferred format would be ([_slug, entries]) => ({....
we added a rule somewhere for linter to ignore args prefixed with an underscore, which is a common convention. Leaving an empty space kind of hides what's available, though not a huge issue
| setTimeout( | ||
| () => attemptSubmission(survey, surveyID, retries + 1), | ||
| retryDelay, | ||
| const newStoredData: StoredSurveyData = { |
There was a problem hiding this comment.
Non-blocking, but worth fixing: a failed local write currently retries a successful POST
The success bookkeeping here (setStoredData / localStorage.setItem) sits inside the same try whose catch retries the request. If the storage write throws — quota exceeded, or storage blocked/disabled in the browser — a successful submission is treated as a failure: the same payload is POSTed up to 3 more times, and the hook can end on status: 'error' even though the server accepted it (duplicate rows depending on whether the endpoint is idempotent on id).
Keeping the write in its own try means the retry path only ever covers the network call:
| const newStoredData: StoredSurveyData = { | |
| const newStoredData: StoredSurveyData = { | |
| answers: survey.answers, | |
| surveyID, | |
| }; | |
| setStoredData(newStoredData); | |
| try { | |
| localStorage.setItem( | |
| LOCAL_STORAGE_KEY, | |
| JSON.stringify(newStoredData), | |
| ); | |
| } catch (storageError) { | |
| // POST already succeeded - don't retry it because we couldn't | |
| // persist locally (quota / blocked storage). | |
| console.error('Failed to persist survey submission:', storageError); | |
| } | |
| setStatus('success'); | |
| console.log('Survey submitted successfully:', surveyID); |
| return data ? JSON.parse(data) : null; | ||
| }, | ||
| ); | ||
| const [storedData, setStoredData] = useState<StoredSurveyData | null>(() => { |
There was a problem hiding this comment.
Non-blocking: an unguarded JSON.parse can crash the results page
If survey_submission holds malformed or legacy JSON (or localStorage access itself throws in a locked-down browser), this throws during the initial render rather than degrading to "no stored submission" — the whole Result page fails to render. Folding the read into a try keeps a bad local value from taking the page down. Returning null is enough; clearing the bad key is optional and needs its own guard, since access can throw in the same cases.
| const [storedData, setStoredData] = useState<StoredSurveyData | null>(() => { | |
| const [storedData, setStoredData] = useState<StoredSurveyData | null>(() => { | |
| try { | |
| const data = localStorage.getItem(LOCAL_STORAGE_KEY); | |
| return data ? JSON.parse(data) : null; | |
| } catch (error) { | |
| // Corrupted or legacy value, or storage unavailable - start fresh | |
| // instead of throwing during render. | |
| console.error('Failed to read stored survey submission:', error); | |
| return null; | |
| } | |
| }); |
could probably move the safe parsing into a safeParse util - in same file probably ok
2 participants
DEV-2478
This story focuses on configuring the Wildfire app for eslint and addressing all the lint erros.
Summary by Sourcery
Configure ESLint for the Wildfire app and bring its codebase into compliance while strengthening related type safety and runtime handling.
Bug Fixes:
Enhancements:
Build: