Add PvP/PvE mode switching and Sentry integration#4
Conversation
- Fetch Tarkov data by game mode and cache both mode variants - Sync completed tasks and objectives across PvP/PvE equivalents - Add coverage for profile, progress view, and API mode handling
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 22 minutes and 3 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/LazyLoadErrorBoundary.tsx (1)
1-27:⚠️ Potential issue | 🔴 Critical
React.ErrorInfois unresolved —Reactnamespace is not imported, causing a TypeScript compilation error.Line 25 references
React.ErrorInfobut the file only imports{ Component, ReactNode }from"react". With TypeScript configured forjsx: "react-jsx"(modern JSX transform), the React namespace is not automatically available for type references. This causes an unresolved name error during compilation.Import
ErrorInfoas a type instead:Fix
-import { Component, ReactNode } from "react"; +import { Component, type ErrorInfo, type ReactNode } from "react"; import * as Sentry from "@sentry/react"; @@ - componentDidCatch(error: Error, errorInfo: React.ErrorInfo) { + componentDidCatch(error: Error, errorInfo: ErrorInfo) { Sentry.captureReactException(error, errorInfo); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/LazyLoadErrorBoundary.tsx` around lines 1 - 27, The TypeScript error is caused by using the React namespace type React.ErrorInfo without importing it; update the import from "react" to include the ErrorInfo type (e.g., add ErrorInfo to the named imports) and then change the componentDidCatch signature to use the imported ErrorInfo type (componentDidCatch(error: Error, errorInfo: ErrorInfo)) in the LazyLoadErrorBoundary class so the type resolves correctly; ensure you only import it as a type if your linter/tsconfig requires.
🧹 Nitpick comments (17)
package.json (1)
9-17: Heads up:build:devuses--mode preview, which is deliberately tied to Sentry sampling.
deploy:devrunsbuild:dev(modepreview) to deploy to thedevbranch, whilesrc/instrument.tskeys traces sampling offimport.meta.env.MODE(development=1.0,preview=0.2, default0.1). The naming is fine but easy to misread as a typo — consider a short comment inpackage.jsonor a README note tying the script names to the Sentry environments to avoid future "fixes" that drop sampling intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 9 - 17, Add a brief note explaining that the "build:dev" npm script intentionally passes "--mode preview" (and that "deploy:dev" invokes it) because src/instrument.ts reads import.meta.env.MODE to set Sentry sampling (development=1.0, preview=0.2, default=0.1); add this comment either inline near the "build:dev"/"deploy:dev" scripts in package.json or as a short entry in README so future contributors won't change the mode thinking it's a typo.scripts/task-cli.ts (1)
71-89: Optional: parameterisegameModeso the CLI can also inspect the PvE task set.The CLI is now hardwired to
regular. Given the PR makes the main app mode-aware, it would be handy to surface a--mode <regular|pve>(or env var) flag here so the same dev tooling can introspect both task sets without code edits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/task-cli.ts` around lines 71 - 89, The GraphQL query in scripts/task-cli.ts is hardcoded to gameMode: regular; update the CLI to accept a --mode (or MODE env var) and inject its value into the query template instead of the fixed "regular" string. Parse the flag (or fallback to process.env.MODE and default to "regular"), validate allowed values ("regular" or "pve"), then build the query by substituting the chosen mode into the existing query constant (refer to the const query) or by constructing the query string where the request is made so the tasks(...) call uses the runtime mode. Ensure invalid modes are rejected with a clear error message and that the default behavior remains "regular".src/main.tsx (1)
17-19: Minor: bare-text fallback UI.
<p>Something went wrong</p>is a noticeably less polished fallback thanLazyLoadErrorBoundary's. Since this is the last-resort root boundary, consider rendering at least a heading + reload button (or reusingLazyLoadErrorBoundary's fallback) so a top-level render error doesn't drop the user onto an unstyled paragraph.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.tsx` around lines 17 - 19, Replace the bare-text fallback used in the Sentry.ErrorBoundary (the fallback prop on Sentry.ErrorBoundary in src/main.tsx) with a more robust root-level UI: either reuse the existing LazyLoadErrorBoundary's fallback component or create a simple fallback that renders a heading (e.g., "Something went wrong") and a reload button that calls window.location.reload(), so top-level render errors don't drop the user onto an unstyled paragraph.src/components/CheckListView.tsx (1)
88-90: Name-only suffix stripping is fragile — consider matching bybuildLogicalTaskKeyinstead.
normalizeModeVariantTaskNameonly knows about the literal[PVP ZONE]/[PVE ZONE]suffix. Any other variant marker (different bracket text, spacing differences, future zone tags) silently falls back to "no match" and the selection gets cleared (line 593). The codebase already hasbuildLogicalTaskKey/logicalTaskGroupsByTaskId(imported at lines 82-86) which is the canonical way to map across variants — using that for selection recovery would be sturdier than string normalisation:// Pseudocode – store the logical key when a task is selected, then on // recovery look up the new variant via logicalTaskGroupsByTaskId.Not blocking — current behaviour is "best effort recover, otherwise clear" — but worth a follow-up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/CheckListView.tsx` around lines 88 - 90, The current normalizeModeVariantTaskName function is too brittle because it strips only literal "[PVP ZONE]"/"[PVE ZONE]" suffixes; change the selection-recovery logic to use the canonical mapping instead: when a task is selected, record its logical key via buildLogicalTaskKey (or store the logical key lookup from logicalTaskGroupsByTaskId), and on recovery/lookup use logicalTaskGroupsByTaskId to find the corresponding current variant task id rather than relying on normalizeModeVariantTaskName string stripping; update any code that calls normalizeModeVariantTaskName for selection fallback to first attempt mapping via buildLogicalTaskKey + logicalTaskGroupsByTaskId and only fallback to the existing name-based normalization as a last resort.vite.config.ts (1)
8-18: Consider gatingsentryVitePluginonSENTRY_AUTH_TOKENbeing present.The plugin is currently configured unconditionally for every Vite invocation (dev, preview, production builds, fork PR builds). When
SENTRY_AUTH_TOKENis absent,@sentry/vite-pluginskips source-map upload but still emits warnings and may trigger telemetry pings. Gating the plugin avoids unnecessary noise and accidental telemetry on local, development, and fork builds:♻️ Proposed refactor
import { sentryVitePlugin } from "@sentry/vite-plugin"; export default defineConfig({ plugins: [ react(), - sentryVitePlugin({ - org: process.env.SENTRY_ORG, - project: process.env.SENTRY_PROJECT, - authToken: process.env.SENTRY_AUTH_TOKEN, - reactComponentAnnotation: { - enabled: true, - }, - }), + process.env.SENTRY_AUTH_TOKEN + ? sentryVitePlugin({ + org: process.env.SENTRY_ORG, + project: process.env.SENTRY_PROJECT, + authToken: process.env.SENTRY_AUTH_TOKEN, + reactComponentAnnotation: { enabled: true }, + telemetry: false, + }) + : null, ],Your
build.sourcemap = "hidden"configuration is correct — production bundles will ship maps as separate.mapfiles without a//# sourceMappingURLcomment, preventing deployed JS from referencing them. However, verify that your Cloudflare Pages configuration does not publicly serve the.mapfiles, or addfilesToDeleteAfterUploadto the plugin config to automatically remove them after upload so they remain inaccessible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vite.config.ts` around lines 8 - 18, The sentryVitePlugin is being instantiated unconditionally in the plugins array which causes warnings/telemetry when SENTRY_AUTH_TOKEN is missing; modify the Vite config to only push/include sentryVitePlugin into the plugins array when process.env.SENTRY_AUTH_TOKEN is present (optionally also check SENTRY_ORG/SENTRY_PROJECT), so local/dev/fork builds skip the plugin entirely; while changing this, keep the existing react() entry and ensure build.sourcemap remains "hidden" and consider adding filesToDeleteAfterUpload to the sentryVitePlugin config if you want uploaded .map files removed after upload.src/services/__tests__/tarkovApi.spec.ts (1)
31-477: Mode-aware cache and query coverage looks comprehensiveThe new tests appropriately verify:
gameMode: regular/gameMode: pveare present in the GraphQL request body- Per-mode cache isolation (
saveCombinedCache(_, "regular"|"pve"))- Legacy single-key cache only acts as a fallback for
regular, neverpveOne small thought: the existing
"should detect stale cache"and"should handle corrupted cache"tests still write directly toAPI_CACHE_KEY(the legacy key). After this PR they're effectively testing the legacy fallback path rather than the v3 path; consider adding parallel cases that write tobuildCombinedCacheKey("regular")/("pve")so the v3 fresh/stale/corrupt detection paths are exercised too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/tarkovApi.spec.ts` around lines 31 - 477, The legacy-key tests still only exercise the old API_CACHE_KEY fallback; update the stale/corrupted-cache tests to also write test payloads (fresh, stale timestamp, and invalid JSON) into the per-mode v3 keys returned by buildCombinedCacheKey("regular") and buildCombinedCacheKey("pve") and assert isCombinedCacheFresh, loadCombinedCache, and related behavior for each mode; ensure you still keep the legacy-key assertions to verify fallback behavior, and reference buildCombinedCacheKey, loadCombinedCache, isCombinedCacheFresh, and API_CACHE_KEY when locating the tests to modify.src/instrument.ts (2)
14-14: Hardcoded Sentry DSN — consider sourcing from envWhile Sentry DSNs are technically public client identifiers, hardcoding it makes it impossible for forks/contributors to opt out, and CI/test runs will emit events into your production project. Reading from
import.meta.env.VITE_SENTRY_DSN(with a no-op when undefined) avoids both.♻️ Suggested change
-Sentry.init({ - dsn: "https://5bf241dff082ead8c4ca29b79789154c@o4509849192824832.ingest.de.sentry.io/4511282185044048", - environment: sentryEnvironment, +const sentryDsn = import.meta.env.VITE_SENTRY_DSN; +if (!sentryDsn) { + // No DSN configured – skip Sentry init (e.g. local dev / forks / CI) +} +Sentry.init({ + dsn: sentryDsn, + enabled: Boolean(sentryDsn), + environment: sentryEnvironment,As per coding guidelines: "Never commit secrets; keep network logic in
services/and avoid embedding keys in UI code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instrument.ts` at line 14, Replace the hardcoded Sentry DSN in the dsn property with an environment-sourced value and guard initialization when absent: read the DSN from import.meta.env.VITE_SENTRY_DSN (use undefined/no-op when it's falsy) and only pass it into the Sentry init/config where the dsn property is currently set (the dsn property in instrument.ts); ensure the code avoids emitting events when the env var is not provided by gating Sentry setup on the presence of that variable.
23-23:tracePropagationTargetsonly includes"localhost"— distributed tracing headers won't be attached to requests toapi.tarkov.dev, so server/client correlation breaks outside local development.Add your production API domain to enable end-to-end tracing:
♻️ Suggested change
- tracePropagationTargets: ["localhost"], + tracePropagationTargets: ["localhost", /^https:\/\/api\.tarkov\.dev/],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instrument.ts` at line 23, tracePropagationTargets currently contains only "localhost", so tracing headers won't propagate to production; update the tracePropagationTargets array (in src/instrument.ts) to include your production API domain "api.tarkov.dev" alongside "localhost" (e.g., tracePropagationTargets: ["localhost", "api.tarkov.dev"]) so distributed tracing works end-to-end; if you prefer environment-specific config, make tracePropagationTargets driven by env and include "api.tarkov.dev" in non-dev environments.src/utils/__tests__/profile.spec.ts (1)
1-50: Tests look reasonable and cover the maingameModepathsCoverage of normalization-on-load, default-on-create, and persistence-on-update is solid. Two small optional improvements you might consider later:
- The
PROFILES_KEYliteral is duplicated fromsrc/utils/profile.ts— if that key ever changes the spec silently passes; exporting/importing the constant would prevent drift.- Consider adding a case for a stored profile with a junk
gameModevalue (e.g."PVP") being normalized to"regular", which is the more interesting branch ofnormalizeGameMode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/__tests__/profile.spec.ts` around lines 1 - 50, Export the PROFILES_KEY constant from src/utils/profile.ts and import it into the test (replace the hardcoded PROFILES_KEY string in profile.spec.ts) to avoid drift, and add an extra test case in profile.spec.ts that seeds localStorage with a profile whose gameMode is an unexpected value (e.g., "PVP") then calls ensureProfiles() and asserts the stored and returned profile.gameMode equals "regular" to cover the normalizeGameMode branch; reference symbols: PROFILES_KEY, normalizeGameMode, ensureProfiles, createProfile, updateProfileGameMode, getProfiles.src/components/TaskDeskView.tsx (2)
79-80: Consider hoisting/sharingnormalizeModeVariantTaskNameThe AI summary mentions
CheckListView.tsxmirrors the same selection/search normalization. If the same regex is being duplicated there, extracting this into a shared helper (e.g.@/utils/taskVariants.tsnext tobuildLogicalTaskKey) would prevent the two copies from drifting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/TaskDeskView.tsx` around lines 79 - 80, normalizeModeVariantTaskName is duplicated elsewhere (e.g., CheckListView) and should be hoisted to a shared helper to avoid drift; create a new utility (e.g., taskVariants helper next to buildLogicalTaskKey), move the regex/normalization logic currently in normalizeModeVariantTaskName into that helper as an exported function, then import and use the shared function from TaskDeskView (replace the local normalizeModeVariantTaskName) and from CheckListView so both components reference the same implementation.
587-594: Auto-expand-all on search runs on everyallGroupNameschangeBecause this effect depends on
allGroupNames, any time the filtered set of groups changes whilesearchTermis non-empty (e.g. flippingshowCompleted, toggling a focus, switching modes), all groups will get re-expanded — which can override a user's explicit collapse during a search session. The early-out vianext.size === prev.lengthonly fires when all group names are already expanded, so partial collapses get overwritten on the next dependency change.If that's intentional ("any search-with-filtered-set should be fully expanded"), feel free to ignore. Otherwise consider gating on a "search just changed" signal rather than on
allGroupNames.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/TaskDeskView.tsx` around lines 587 - 594, The effect that auto-expands groups (useEffect that calls setExpandedGroups) currently runs on changes to allGroupNames and searchTerm, causing expansions whenever filters change; restrict it to only trigger when the search term itself changes from empty to non-empty (i.e., when a new search starts) rather than on every allGroupNames update. Modify the effect logic so it uses searchTerm as the primary trigger (e.g., depend on searchTerm only or track previousSearchTerm with a ref) and bail out unless searchTerm transitioned from empty to non-empty; keep the existing checks for empty searchTerm and allGroupNames length and continue to call setExpandedGroups (the same function name) only when the search just started.docs/PVP_PVE_TASK_OBJECTIVE_API_CHECK.md (1)
1-62: Helpful audit doc — small consistency noteThe methodology, results, and worked examples are clear and the conclusion ("compare by mode-aware task set first") matches the implementation direction in
taskProgressView.tsandtarkovApi.ts. Two tiny optional polish suggestions:
- Consider adding the exact GraphQL field selection that was diffed (you list the fields, but a full minimal query helps future audits reproduce the check).
- Mention whether the audit was rerun against the API used by tests (or if it's a one-off snapshot) so readers know the freshness expectations.
The LanguageTool "loose punctuation" hint on line 16 is a false positive — that's standard Markdown list formatting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/PVP_PVE_TASK_OBJECTIVE_API_CHECK.md` around lines 1 - 62, The doc is missing the exact GraphQL field selection and a freshness note—add a minimal reproducible GraphQL query snippet showing the fields used (description, maps { name }, TaskObjectiveItem items/count/foundInRaid, TaskObjectiveShoot count, TaskObjectivePlayerLevel playerLevel) and a short statement whether this audit was rerun against the same API endpoint used by tests (or if it’s a one-off snapshot) and how often it should be rechecked; reference the implementation files taskProgressView.ts and tarkovApi.ts so readers can map fields to code, and add a brief parenthetical noting the LanguageTool "loose punctuation" hint is a false positive for the Markdown list.src/utils/taskProgressView.ts (2)
61-69: Edge case: legacy index parsing with-chars.
getLegacyObjectiveIndexdoesNumber(legacyObjectiveKey.slice(prefix.length)). Tarkov task IDs are 24-char hex strings so${taskId}-is unambiguous in practice, but if the legacy key format ever expands beyond<taskId>-<index>(e.g.,<taskId>-<objectiveType>-<index>)Number(...)will quietly returnNaN, whichNumber.isIntegercorrectly rejects — so it fails closed. No bug today, but consider a brief comment noting the assumed format so future changes don't subtly break equivalence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/taskProgressView.ts` around lines 61 - 69, The function getLegacyObjectiveIndex silently assumes legacyObjectiveKey follows the exact "<taskId>-<index>" format so future expansions like "<taskId>-<type>-<index>" could break; add a brief comment above getLegacyObjectiveIndex stating this assumed format (e.g., legacy key must be `${taskId}-${index}` as a decimal integer) and note why the current implementation parses with slice+Number and rejects non-integer values, so maintainers know the limitation and can update parsing (or replace with a regex) if the format changes.
107-143: LGTM, with a minor performance note.The cross-mode expansion is correct. One thing to keep in mind: this runs in
useMemoinApp.tsxwithO(modes × tasks × objectives × |equivalent group| × objectives')and is recomputed whenevercompletedTaskObjectives,modeTasksByMode,knownTasksById, orlogicalTaskIdsByTaskIdchanges. With ~hundreds of tasks across two modes this is fine, but if it ever shows up as jank, you can pre-build a reverse mapobjectiveKey → equivalentObjectiveKey[]once pertasksByMode/logicalTaskIdsByTaskIdchange and reuse it on eachcompletedTaskObjectivesmutation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/taskProgressView.ts` around lines 107 - 143, The function expandCompletedTaskObjectives currently recomputes equivalent groups for every completedTaskObjectives change which can be costly; to speed this up, precompute a reverse map from each objective key to its equivalent keys once whenever tasksByMode or logicalTaskIdsByTaskId changes and then use that map inside expandCompletedTaskObjectives to expand completed keys quickly. Concretely: build a preprocessing step (e.g., createObjectiveEquivalentsMap(tasksByMode, tasksById, logicalTaskIdsByTaskId) that returns a Map<string, string[]> keyed by objectiveKey and legacyKey using the logic in getEquivalentTaskObjectiveKeys/getEquivalentLegacyTaskObjectiveKeys and buildTaskObjectiveKeys/buildLegacyTaskObjectiveKey, then modify expandCompletedTaskObjectives to look up equivalent keys in that map (instead of recomputing per task/objective) and union them into expanded. Ensure the map is invalidated/rebuilt when tasksByMode, tasksById, or logicalTaskIdsByTaskId change.src/services/tarkovApi.ts (2)
651-695: TwoDate.now()calls + non-atomic split write can desync freshness.
saveCombinedCachecallsDate.now()twice (once for shared, once for task) and thenisCombinedCacheFreshrequires both timestamps withinttlMs. In the rare case where the secondsetItemis delayed (task-only cache is much larger and might trigger a slow write), or the writes interleave with another tab updating only one key, the two timestamps drift. Capturing one timestamp and reusing it makes freshness deterministic.♻️ Suggested change
+ const now = Date.now(); // Save shared data to localStorage (smaller, shared across modes) if (!localStorageQuotaExceeded) { try { const sharedData: SharedCacheData = { - updatedAt: Date.now(), + updatedAt: now, ... }; ... const taskData: TaskOnlyCache = { - updatedAt: Date.now(), + updatedAt: now, payload: { tasks: payload.tasks }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/tarkovApi.ts` around lines 651 - 695, In saveCombinedCache, avoid using two Date.now() calls and non-atomic writes that can desync freshness; capture a single timestamp (e.g., const now = Date.now()) once at the start of the function and reuse it for both sharedData.updatedAt and taskData.updatedAt before calling localStorage.setItem for SHARED_CACHE_KEY and buildCombinedCacheKey(normalizedGameMode) so both stored entries share the identical updatedAt value; keep the existing try/catch and quota handling while only changing the timestamp creation and assignment to make freshness deterministic.
602-609: ConfusingGameMode | numberoverload for backwards compatibility.Treating a numeric first argument as the TTL is a runtime trick that defeats type-checking and trips up readers. Existing callers in this PR (
App.tsx) all pass aGameMode, so the overload only buys back-compat with hypothetical external callers that never existed before this PR. Recommend dropping the runtime branch and tightening the signature to(gameMode?: GameMode, ttlMs?: number).♻️ Proposed simplification
export function isCombinedCacheFresh( - gameMode: GameMode | number = DEFAULT_GAME_MODE, + gameMode: GameMode = DEFAULT_GAME_MODE, ttlMs: number = API_CACHE_TTL_MS, ): boolean { - if (typeof gameMode === "number") { - ttlMs = gameMode; - gameMode = DEFAULT_GAME_MODE; - } const normalizedGameMode = normalizeGameMode(gameMode);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/tarkovApi.ts` around lines 602 - 609, The isCombinedCacheFresh function uses a confusing GameMode | number overload and runtime branch that treats a numeric first arg as ttl; change the signature to (gameMode?: GameMode, ttlMs?: number) and remove the runtime branch that checks typeof gameMode === "number"; ensure DEFAULT_GAME_MODE and API_CACHE_TTL_MS are used when args are undefined and update any internal references in isCombinedCacheFresh to rely on the two-parameter form (gameMode, ttlMs) only so callers in App.tsx stay type-safe and code is clearer.src/App.tsx (1)
1669-1672: Redundant initial fetch right afterinitcompletes.When
initfinishes it has already populated mode-scoped data and freshly written the cache. As soon as it setshasInitializedRef.current = trueandsetIsLoading(false), this effect fires and callsloadGameModeData(activeProfileGameMode)for the same mode.loadGameModeDatawill re-read the cache, re-applyCombinedData(re-rendering all consumers), checkisCombinedCacheFreshand only then early-return — that's an avoidable re-set ofallTasks/achievements/hideoutStationsand a state churn on first render.Consider gating this effect on something like a
lastLoadedGameModeRefso it skips when the active mode matches whatinitalready loaded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` around lines 1669 - 1672, The effect that calls loadGameModeData re-fetches immediately after init finishes, causing redundant cache reads and state churn; add a ref like lastLoadedGameModeRef and set it when init populates mode-scoped data (where hasInitializedRef.current is set) and also when loadGameModeData successfully loads a mode, then update the useEffect condition to return early if lastLoadedGameModeRef.current === activeProfileGameMode so the effect skips calling loadGameModeData for the mode already loaded; reference useEffect, hasInitializedRef, activeProfileGameMode, loadGameModeData and the init code path that sets hasInitializedRef to locate changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/App.tsx`:
- Around line 718-742: The current catch block permanently persists a PvE→PvP
fallback by calling updateProfileGameMode and setProfiles on the first transient
failure; change it to only perform an in-memory fallback (call
setActiveProfileGameMode(DEFAULT_GAME_MODE) and show the toast) and remove the
updateProfileGameMode(activeProfileId, DEFAULT_GAME_MODE) and
setProfiles(getProfiles()) calls so the profile isn't mutated on a single error.
If you want persistence only after repeated failures, implement a small failure
counter keyed by activeProfileId (e.g., pveFailureCount map) incremented on each
non-cached PvE error and only call updateProfileGameMode once the count exceeds
a threshold (reset the counter on success); also consider inspecting the error
shape before deciding messaging so the toast accurately reflects the root cause.
In `@src/components/app-sidebar.tsx`:
- Around line 355-397: Buttons in the GAME_MODES map currently convey selection
only by color; add accessible toggle semantics by setting
aria-pressed={activeGameMode === gameMode} (and/or aria-current="true" when
selected) on the button rendered inside the GAME_MODES loop so screen readers
know which mode is active, keep the existing onClick/onUpdateGameMode and
disabled handling, and ensure the spinner SVG used when isSwitchingMode is true
includes aria-hidden="true" and focusable="false" (and no accessible child text)
so assistive tech ignores the decorative spinner; reference the mapped elements
using GAME_MODES, GAME_MODE_LABELS, activeGameMode, isSwitchingMode, and the
onUpdateGameMode call to locate where to apply these attributes.
- Line 363: The button's disabled prop currently uses only isGameModeLoading,
which allows a brief race when isSwitchingMode is true but isGameModeLoading is
false; update the disabled condition in the button render inside AppSidebar
(where disabled={isGameModeLoading}) to disable when either flag is set (e.g.,
disabled={isGameModeLoading || isSwitchingMode}) or use a single combined state
(like isSwitching) checked by the button; ensure you update the render where
onUpdateGameMode is invoked so the button cannot be clicked while the switch is
in progress.
In `@src/components/CheckListView.tsx`:
- Around line 563-570: The current useEffect that auto-expands allGroupNames
whenever searchTerm is non-empty will override user-collapsed groups on
subsequent re-renders; change it to only expand on the leading edge of the
search (when searchTerm transitions from empty to non-empty) by adding a ref
like prevSearchTermRef, read prev value inside the useEffect and only run the
expansion logic when prev was empty and searchTerm is now non-empty, then update
the ref to the current searchTerm; keep the existing setExpandedGroups(next)
logic but guard it with this transition check so manual collapses while search
is non-empty are preserved.
In `@src/main.tsx`:
- Around line 10-14: The createRoot call is passing React 19-only options
onUncaughtError and onCaughtError which will be ignored under react ^18.3.1;
update createRoot(...) to either remove onUncaughtError and onCaughtError and
rely on Sentry.ErrorBoundary / componentDidCatch plus global listeners (keeping
only onRecoverableError with Sentry.reactErrorHandler), or keep the two options
but add a clear comment noting they are React 19-only (future-proofing) so
reviewers understand they are intentionally no-ops on React 18; touch the
createRoot invocation and document the choice, referencing createRoot,
onUncaughtError, onCaughtError, onRecoverableError, Sentry.reactErrorHandler,
and Sentry.ErrorBoundary/componentDidCatch.
In `@src/services/tarkovApi.ts`:
- Around line 27-29: The module-level boolean localStorageQuotaExceeded is
sticky and prevents recovery after a QuotaExceededError; update
saveCombinedCache (and any other places that set localStorageQuotaExceeded) to
make the guard per-write or time-bound: replace the boolean with a cooldown
timestamp (e.g., localStorageQuotaExceededUntil) or reset the flag on any
successful localStorage.setItem, and ensure QuotaExceededError handling only
disables writes until the cooldown expires (or until a later successful write
clears it) so future saveCombinedCache attempts will retry storage once the
quota may have freed up. Ensure you update any checks and assignments that
reference localStorageQuotaExceeded to use the new timestamp logic (or to reset
on success) and keep QuotaExceededError handling limited to the write attempt
that failed.
- Around line 591-600: The fallback is returning a TaskOnlyCache as if it were a
CombinedCachePayload; update the code around
readStoredCache(buildCombinedCacheKey(normalizedGameMode)) in the function that
currently returns modeCache.payload so it first validates the payload shape
(e.g., ensure payload.collectorItems, payload.achievements?.data?.achievements
and payload.hideoutStations exist and are the right types) before
casting/returning as CombinedCachePayload; if the checks fail, fall through to
the DEFAULT_GAME_MODE/API_CACHE_KEY branch or return null instead of returning a
malformed object. Use the existing helpers readStoredCache,
buildCombinedCacheKey, and the CombinedCachePayload/TaskOnlyCache type
distinctions to implement the runtime guard so downstream callers like
applyCombinedData / setApiCollectorItems / setAchievements never receive an
incomplete payload.
In `@src/utils/indexedDB.ts`:
- Around line 733-774: TaskStorage exposes loadTaskCache and isTaskCacheFresh
but they are unused and isTaskCacheFresh hardcodes ttl; either wire them into
the cache load path (use loadTaskCache inside the task-loading routine as an
IndexedDB fallback when localStorage fails, and call isTaskCacheFresh to gate
using that fallback) or remove both methods; if wiring, import and reuse the
canonical TTL constant (API_CACHE_TTL_MS from tarkovApi.ts) inside
isTaskCacheFresh instead of the hardcoded 1000*60*30, and ensure saveTaskCache
remains compatible with the same key format (`tasks::${gameMode}`) so
tarkovApi.ts:saveCombinedCache and TaskStorage methods operate on the same cache
entries.
---
Outside diff comments:
In `@src/components/LazyLoadErrorBoundary.tsx`:
- Around line 1-27: The TypeScript error is caused by using the React namespace
type React.ErrorInfo without importing it; update the import from "react" to
include the ErrorInfo type (e.g., add ErrorInfo to the named imports) and then
change the componentDidCatch signature to use the imported ErrorInfo type
(componentDidCatch(error: Error, errorInfo: ErrorInfo)) in the
LazyLoadErrorBoundary class so the type resolves correctly; ensure you only
import it as a type if your linter/tsconfig requires.
---
Nitpick comments:
In `@docs/PVP_PVE_TASK_OBJECTIVE_API_CHECK.md`:
- Around line 1-62: The doc is missing the exact GraphQL field selection and a
freshness note—add a minimal reproducible GraphQL query snippet showing the
fields used (description, maps { name }, TaskObjectiveItem
items/count/foundInRaid, TaskObjectiveShoot count, TaskObjectivePlayerLevel
playerLevel) and a short statement whether this audit was rerun against the same
API endpoint used by tests (or if it’s a one-off snapshot) and how often it
should be rechecked; reference the implementation files taskProgressView.ts and
tarkovApi.ts so readers can map fields to code, and add a brief parenthetical
noting the LanguageTool "loose punctuation" hint is a false positive for the
Markdown list.
In `@package.json`:
- Around line 9-17: Add a brief note explaining that the "build:dev" npm script
intentionally passes "--mode preview" (and that "deploy:dev" invokes it) because
src/instrument.ts reads import.meta.env.MODE to set Sentry sampling
(development=1.0, preview=0.2, default=0.1); add this comment either inline near
the "build:dev"/"deploy:dev" scripts in package.json or as a short entry in
README so future contributors won't change the mode thinking it's a typo.
In `@scripts/task-cli.ts`:
- Around line 71-89: The GraphQL query in scripts/task-cli.ts is hardcoded to
gameMode: regular; update the CLI to accept a --mode (or MODE env var) and
inject its value into the query template instead of the fixed "regular" string.
Parse the flag (or fallback to process.env.MODE and default to "regular"),
validate allowed values ("regular" or "pve"), then build the query by
substituting the chosen mode into the existing query constant (refer to the
const query) or by constructing the query string where the request is made so
the tasks(...) call uses the runtime mode. Ensure invalid modes are rejected
with a clear error message and that the default behavior remains "regular".
In `@src/App.tsx`:
- Around line 1669-1672: The effect that calls loadGameModeData re-fetches
immediately after init finishes, causing redundant cache reads and state churn;
add a ref like lastLoadedGameModeRef and set it when init populates mode-scoped
data (where hasInitializedRef.current is set) and also when loadGameModeData
successfully loads a mode, then update the useEffect condition to return early
if lastLoadedGameModeRef.current === activeProfileGameMode so the effect skips
calling loadGameModeData for the mode already loaded; reference useEffect,
hasInitializedRef, activeProfileGameMode, loadGameModeData and the init code
path that sets hasInitializedRef to locate changes.
In `@src/components/CheckListView.tsx`:
- Around line 88-90: The current normalizeModeVariantTaskName function is too
brittle because it strips only literal "[PVP ZONE]"/"[PVE ZONE]" suffixes;
change the selection-recovery logic to use the canonical mapping instead: when a
task is selected, record its logical key via buildLogicalTaskKey (or store the
logical key lookup from logicalTaskGroupsByTaskId), and on recovery/lookup use
logicalTaskGroupsByTaskId to find the corresponding current variant task id
rather than relying on normalizeModeVariantTaskName string stripping; update any
code that calls normalizeModeVariantTaskName for selection fallback to first
attempt mapping via buildLogicalTaskKey + logicalTaskGroupsByTaskId and only
fallback to the existing name-based normalization as a last resort.
In `@src/components/TaskDeskView.tsx`:
- Around line 79-80: normalizeModeVariantTaskName is duplicated elsewhere (e.g.,
CheckListView) and should be hoisted to a shared helper to avoid drift; create a
new utility (e.g., taskVariants helper next to buildLogicalTaskKey), move the
regex/normalization logic currently in normalizeModeVariantTaskName into that
helper as an exported function, then import and use the shared function from
TaskDeskView (replace the local normalizeModeVariantTaskName) and from
CheckListView so both components reference the same implementation.
- Around line 587-594: The effect that auto-expands groups (useEffect that calls
setExpandedGroups) currently runs on changes to allGroupNames and searchTerm,
causing expansions whenever filters change; restrict it to only trigger when the
search term itself changes from empty to non-empty (i.e., when a new search
starts) rather than on every allGroupNames update. Modify the effect logic so it
uses searchTerm as the primary trigger (e.g., depend on searchTerm only or track
previousSearchTerm with a ref) and bail out unless searchTerm transitioned from
empty to non-empty; keep the existing checks for empty searchTerm and
allGroupNames length and continue to call setExpandedGroups (the same function
name) only when the search just started.
In `@src/instrument.ts`:
- Line 14: Replace the hardcoded Sentry DSN in the dsn property with an
environment-sourced value and guard initialization when absent: read the DSN
from import.meta.env.VITE_SENTRY_DSN (use undefined/no-op when it's falsy) and
only pass it into the Sentry init/config where the dsn property is currently set
(the dsn property in instrument.ts); ensure the code avoids emitting events when
the env var is not provided by gating Sentry setup on the presence of that
variable.
- Line 23: tracePropagationTargets currently contains only "localhost", so
tracing headers won't propagate to production; update the
tracePropagationTargets array (in src/instrument.ts) to include your production
API domain "api.tarkov.dev" alongside "localhost" (e.g.,
tracePropagationTargets: ["localhost", "api.tarkov.dev"]) so distributed tracing
works end-to-end; if you prefer environment-specific config, make
tracePropagationTargets driven by env and include "api.tarkov.dev" in non-dev
environments.
In `@src/main.tsx`:
- Around line 17-19: Replace the bare-text fallback used in the
Sentry.ErrorBoundary (the fallback prop on Sentry.ErrorBoundary in src/main.tsx)
with a more robust root-level UI: either reuse the existing
LazyLoadErrorBoundary's fallback component or create a simple fallback that
renders a heading (e.g., "Something went wrong") and a reload button that calls
window.location.reload(), so top-level render errors don't drop the user onto an
unstyled paragraph.
In `@src/services/__tests__/tarkovApi.spec.ts`:
- Around line 31-477: The legacy-key tests still only exercise the old
API_CACHE_KEY fallback; update the stale/corrupted-cache tests to also write
test payloads (fresh, stale timestamp, and invalid JSON) into the per-mode v3
keys returned by buildCombinedCacheKey("regular") and
buildCombinedCacheKey("pve") and assert isCombinedCacheFresh, loadCombinedCache,
and related behavior for each mode; ensure you still keep the legacy-key
assertions to verify fallback behavior, and reference buildCombinedCacheKey,
loadCombinedCache, isCombinedCacheFresh, and API_CACHE_KEY when locating the
tests to modify.
In `@src/services/tarkovApi.ts`:
- Around line 651-695: In saveCombinedCache, avoid using two Date.now() calls
and non-atomic writes that can desync freshness; capture a single timestamp
(e.g., const now = Date.now()) once at the start of the function and reuse it
for both sharedData.updatedAt and taskData.updatedAt before calling
localStorage.setItem for SHARED_CACHE_KEY and
buildCombinedCacheKey(normalizedGameMode) so both stored entries share the
identical updatedAt value; keep the existing try/catch and quota handling while
only changing the timestamp creation and assignment to make freshness
deterministic.
- Around line 602-609: The isCombinedCacheFresh function uses a confusing
GameMode | number overload and runtime branch that treats a numeric first arg as
ttl; change the signature to (gameMode?: GameMode, ttlMs?: number) and remove
the runtime branch that checks typeof gameMode === "number"; ensure
DEFAULT_GAME_MODE and API_CACHE_TTL_MS are used when args are undefined and
update any internal references in isCombinedCacheFresh to rely on the
two-parameter form (gameMode, ttlMs) only so callers in App.tsx stay type-safe
and code is clearer.
In `@src/utils/__tests__/profile.spec.ts`:
- Around line 1-50: Export the PROFILES_KEY constant from src/utils/profile.ts
and import it into the test (replace the hardcoded PROFILES_KEY string in
profile.spec.ts) to avoid drift, and add an extra test case in profile.spec.ts
that seeds localStorage with a profile whose gameMode is an unexpected value
(e.g., "PVP") then calls ensureProfiles() and asserts the stored and returned
profile.gameMode equals "regular" to cover the normalizeGameMode branch;
reference symbols: PROFILES_KEY, normalizeGameMode, ensureProfiles,
createProfile, updateProfileGameMode, getProfiles.
In `@src/utils/taskProgressView.ts`:
- Around line 61-69: The function getLegacyObjectiveIndex silently assumes
legacyObjectiveKey follows the exact "<taskId>-<index>" format so future
expansions like "<taskId>-<type>-<index>" could break; add a brief comment above
getLegacyObjectiveIndex stating this assumed format (e.g., legacy key must be
`${taskId}-${index}` as a decimal integer) and note why the current
implementation parses with slice+Number and rejects non-integer values, so
maintainers know the limitation and can update parsing (or replace with a regex)
if the format changes.
- Around line 107-143: The function expandCompletedTaskObjectives currently
recomputes equivalent groups for every completedTaskObjectives change which can
be costly; to speed this up, precompute a reverse map from each objective key to
its equivalent keys once whenever tasksByMode or logicalTaskIdsByTaskId changes
and then use that map inside expandCompletedTaskObjectives to expand completed
keys quickly. Concretely: build a preprocessing step (e.g.,
createObjectiveEquivalentsMap(tasksByMode, tasksById, logicalTaskIdsByTaskId)
that returns a Map<string, string[]> keyed by objectiveKey and legacyKey using
the logic in getEquivalentTaskObjectiveKeys/getEquivalentLegacyTaskObjectiveKeys
and buildTaskObjectiveKeys/buildLegacyTaskObjectiveKey, then modify
expandCompletedTaskObjectives to look up equivalent keys in that map (instead of
recomputing per task/objective) and union them into expanded. Ensure the map is
invalidated/rebuilt when tasksByMode, tasksById, or logicalTaskIdsByTaskId
change.
In `@vite.config.ts`:
- Around line 8-18: The sentryVitePlugin is being instantiated unconditionally
in the plugins array which causes warnings/telemetry when SENTRY_AUTH_TOKEN is
missing; modify the Vite config to only push/include sentryVitePlugin into the
plugins array when process.env.SENTRY_AUTH_TOKEN is present (optionally also
check SENTRY_ORG/SENTRY_PROJECT), so local/dev/fork builds skip the plugin
entirely; while changing this, keep the existing react() entry and ensure
build.sourcemap remains "hidden" and consider adding filesToDeleteAfterUpload to
the sentryVitePlugin config if you want uploaded .map files removed after
upload.
🪄 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: CHILL
Plan: Pro
Run ID: 64e75f3a-c6cb-48ec-b603-de3c5c16225b
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
docs/PVP_PVE_TASK_OBJECTIVE_API_CHECK.mdpackage.jsonscripts/task-cli.tssrc/App.tsxsrc/components/CheckListView.tsxsrc/components/CommandMenu.tsxsrc/components/LazyLoadErrorBoundary.tsxsrc/components/TaskDeskView.tsxsrc/components/app-sidebar.tsxsrc/instrument.tssrc/main.tsxsrc/services/__tests__/tarkovApi.spec.tssrc/services/tarkovApi.tssrc/utils/__tests__/profile.spec.tssrc/utils/__tests__/taskProgressView.spec.tssrc/utils/gameMode.tssrc/utils/indexedDB.tssrc/utils/profile.tssrc/utils/taskProgressView.tsvite.config.ts
1 participant
Summary
Testing
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation