Fix multiple Sentry issues for theninjarpg#1252
Merged
MathiasGruber merged 20 commits intomainfrom Mar 19, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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:
WalkthroughExpanded client-side Sentry/trpc error filtering (Safari/429/JSON/WebGL/wallet), hardened Three.js/WebGL renderer and shader checks/utilities, reworked travel/robbery DB updates with parallel ops and rollback plus avatar defaults, adjusted WS readiness signaling, dependency bumps, and various small server/client refinements. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as rgba(30,144,255,0.5)
participant Server as rgba(34,139,34,0.5)
participant DB as rgba(255,165,0,0.5)
participant Notifications as rgba(199,21,133,0.5)
Client->>Server: POST /travel/robPlayer(request)
Server->>DB: Read states (robber, target, stealth, clan)
par Parallel updates
Server->>DB: Update robber money & prestige
Server->>DB: Update target money & immunity
alt stealth active
Server->>DB: Update stealth break
end
alt clan points needed
Server->>DB: Increment clan points
end
end
alt robber update fails
Server->>DB: Rollback target update
Server-->>Client: Return failure → initiate combat
else target update fails
Server->>DB: Rollback robber update
Server-->>Client: Return failure → initiate combat
else all succeed
Server->>DB: Commit
Server->>Notifications: Notify target
Server-->>Client: Return success with updated money
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant TravelRouter
participant DB
Client->>TravelRouter: robPlayer(userId, location)
TravelRouter->>DB: fetchUser(robber), fetchUser(target)
alt success === true (40% chance)
TravelRouter->>DB: breakStealth (if active) [sequential]
TravelRouter->>DB: robberUpdate (money+, prestige+)
alt robberUpdate.rowsAffected === 0
TravelRouter-->>Client: errorResponse("Failed to update robber")
end
par Parallel writes
TravelRouter->>DB: targetUpdate (money-, robImmunity)
TravelRouter->>DB: clanPointsUpdate (points+1) [if clanId]
end
alt targetUpdate.rowsAffected === 0
TravelRouter->>DB: rollback robber (money-, prestige-)
Note over TravelRouter,DB: ⚠️ clanPointsUpdate NOT rolled back
TravelRouter-->>Client: errorResponse("Failed to update target")
end
TravelRouter-->>Client: success + stolen amount
else success === false (60% chance)
TravelRouter->>DB: breakStealth (if active) [sequential]
TravelRouter->>TravelRouter: initiateBattle()
TravelRouter-->>Client: battleId or failure message
end
Prompt To Fix All With AIThis is a comment left during a code review.
Path: app/src/server/api/routers/travel.ts
Line: 195-213
Comment:
**Clan prestige not rolled back when target update fails**
The `clanPointsUpdate` runs in parallel with `targetUpdate` inside the `Promise.all`. When `targetUpdate.rowsAffected === 0`, only the robber's money and `villagePrestige` are reversed — the clan `points` and `activityPoints` incremented by `clanPointsUpdate` are never undone.
This means the clan receives a permanent +1 point for a robbery that was ultimately rolled back, violating data consistency.
The rollback block needs to also reverse the clan update when `user.clanId` is set:
```
if (targetUpdate && targetUpdate.rowsAffected === 0) {
await Promise.all([
ctx.drizzle
.update(userData)
.set({
money: sql`${userData.money} - ${stolenAmount}`,
villagePrestige: sql`${userData.villagePrestige} - ${ROBBING_VILLAGE_PRESTIGE_GAIN}`,
})
.where(eq(userData.userId, ctx.userId)),
...(user.clanId
? [
ctx.drizzle
.update(clan)
.set({
points: sql`${clan.points} - 1`,
activityPoints: sql`${clan.activityPoints} - 1`,
})
.where(eq(clan.id, user.clanId)),
]
: []),
]);
return errorResponse("Failed to update target's data");
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: app/src/instrumentation-client.ts
Line: 287-292
Comment:
**Breadcrumb check uses `some` instead of matching the most recent fetch**
`isRateLimitJsonParseError` uses `event.breadcrumbs?.some(...)` to check for a 429 status code. This means the filter activates if **any** fetch breadcrumb in the entire session history returned 429, even if that rate-limit occurred many interactions ago and the current JSON parse error has an unrelated cause (e.g., a CDN returning a corrupted response ten minutes after a rate-limit toast).
Compare with `isServerActionError` (line ~407), which correctly uses `fetchBreadcrumbs[fetchBreadcrumbs.length - 1]` — only the **last** fetch breadcrumb — to establish a causal link.
```suggestion
const has429Status = (() => {
const fetchBreadcrumbs = (event.breadcrumbs ?? []).filter(
(b) => b.category === "fetch"
);
const lastFetch = fetchBreadcrumbs[fetchBreadcrumbs.length - 1];
if (!lastFetch) return false;
return (
lastFetch.data?.status_code === 429 || lastFetch.data?.status_code === "429"
);
})();
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: app/src/instrumentation-client.ts
Line: 619-621
Comment:
**Minified identifier pattern is excessively broad**
The regex `/^(Error: )?[A-Z][a-z]?$/` matches **any** 1–2 character message that starts with an uppercase letter: `"No"`, `"Ok"`, `"N"`, `"A"`, `"He"`, etc.
On a mobile browser on the `/combat` or `/travel` page, any native exception with a very short message (e.g., a WebSocket disconnect represented as `"No"`, a null-dereference thrown as `"N"`) would be silently dropped even if it has nothing to do with Three.js.
A tighter pattern, closer to the actual minified identifiers observed (`"Nd"`, `"Od"`, `"Zd"`), would reduce false positives:
```suggestion
const isMinifiedIdentifier = /^(Error: )?[A-Z][a-z]$/.test(message.trim());
```
This still matches the two-character patterns (`Nd`, `Od`, `Zd`) that triggered the Sentry issues while excluding single-character strings like `"N"` or common words like `"No"` and `"Ok"` that could represent real errors.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "Fix feedback" |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/src/layout/Sector.tsx (1)
596-629:⚠️ Potential issue | 🟠 Major
IMG_AVATAR_DEFAULTnever gets used with the current guard.This effect still requires
userData.avatarbefore callingmove(), so users with a missing avatar bail out before reaching the new fallback values on Lines 628-629. That preserves the original "can't move without an avatar" failure mode.Suggested fix
- userData && - userData.avatar + userData🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/layout/Sector.tsx` around lines 596 - 629, The effect currently gates movement on userData.avatar so IMG_AVATAR_DEFAULT never gets used; remove the strict avatar requirement in the initial guard so the effect runs when userData exists (keep checking userData.status inside the effect), i.e. change the condition from "... && userData && userData.avatar" to "... && userData" in the useEffect, leaving the move(...) call as-is so avatar and avatarLight fallback to IMG_AVATAR_DEFAULT when userData.avatar is missing.app/src/server/api/routers/travel.ts (1)
160-203:⚠️ Potential issue | 🔴 CriticalThis rob success path can partially commit and still return an error.
robberUpdate,targetUpdate, clan points, andbreakStealthall run in the samePromise.all(). A failedrobberUpdatecan still leave the victim debited, and a rejected clan/stealth side effect can bubble after both money writes already committed. Run only the two money updates first, verify bothrowsAffectedchecks, then apply side effects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/server/api/routers/travel.ts` around lines 160 - 203, The current Promise.all groups the two money updates with side effects (breakStealth and clan update) so a failure in one can leave partial commits; change the flow in the rob handler to first run only the two money updates via Promise.all on the two ctx.drizzle.update(...) calls that produce robberUpdate and targetUpdate, check robberUpdate.rowsAffected and targetUpdate.rowsAffected and perform the rollback (using the existing ctx.drizzle.update that reverses robber changes) if either fails, and only after both money updates succeed run the side effects (call breakStealth(ctx.drizzle, ctx.userId, user.stealth, false) and the ctx.drizzle.update(...) for clan points) separately and handle their failures independently so they cannot cause money updates to partially commit.app/src/server/api/routers/staff.ts (1)
1192-1207:⚠️ Potential issue | 🟠 MajorClean up
mpvpBattleQueuewhen deleting queued battle users.Batch 10 removes
mpvpBattleUserrows but never prunes emptympvpBattleQueueteams or clears stuckclaiming-*battle IDs.forceAwakealready handles that repair path earlier in this file, so deleting a queued user can still leave stale team state behind.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/server/api/routers/staff.ts` around lines 1192 - 1207, Batch 10 deletes mpvpBattleUser rows but doesn't clean up related mpvpBattleQueue state, leaving empty teams or stuck "claiming-*" battle IDs; update the deletion block to also prune mpvpBattleQueue entries related to the removed user by: remove queue rows where the team owner or any team member equals userId (reference mpvpBattleQueue and mpvpBattleUser), delete or clear mpvpBattleQueue rows whose battleId matches the "claiming-%" pattern for that user's teams, and ensure the same repair logic used in forceAwake is applied (or call the same helper) so empty teams and claiming-* IDs are cleared when deleting a queued user.
🧹 Nitpick comments (2)
app/src/libs/threejs/util.ts (1)
905-915: Keep the shader probe out of the per-frame validity path.
isWebGLContextValid()allocates and deletes a shader on every call, and both render loops now invokeisRendererContextValid(renderer)every animation frame. That makes the safety check part of the hot render path on the exact browsers we're trying to stabilize.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/libs/threejs/util.ts` around lines 905 - 915, The shader creation probe in isWebGLContextValid() is being executed every animation frame (via isRendererContextValid(renderer)), which is expensive; move that expensive probe out of the per-frame path by removing createShader/deleteShader from isWebGLContextValid() and instead perform the shader probe only during context initialization or on demand (e.g., a new probe function like probeShaderSupportOnce or a memoized check with a timestamp/flag), and update isRendererContextValid(renderer) to rely on the lightweight checks in isWebGLContextValid() and the cached probe result or event-driven re-check triggered on context lost/restored; ensure you reference and update the functions isWebGLContextValid() and isRendererContextValid(renderer) and any new probe function/flag used for caching.app/src/server/api/routers/staff.ts (1)
484-489: Remove the extra attribute fetch here.
_userAttributesis never read after this destructure, so this adds an extra database round-trip to every debug clone without changing behavior.
Based on learnings, "Always try to usePromise.all()for parallel database operations to minimize database back-and-forth."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/server/api/routers/staff.ts` around lines 484 - 489, The destructure currently awaits four parallel calls but the third result (_userAttributes) is never used; remove the unnecessary fetchAttributes(ctx.drizzle, ctx.userId) call and adjust the Promise.all to only run the needed three calls (fetchUser(ctx.drizzle, ctx.userId), fetchUser(ctx.drizzle, input.userId), fetchAttributes(ctx.drizzle, input.userId)), updating the destructured variables accordingly so _userAttributes is no longer created.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Line 29: The .hermes/ entry in .gitignore appears unrelated to the Sentry
fixes; either remove that line from the PR or add a brief inline comment in
.gitignore explaining why .hermes/ is being ignored and how it relates to the
work (e.g., "Ignore Hermes cache generated by X build step — not related to
Sentry fixes"). If this change is unrelated, revert the addition and open a
separate PR for ignoring .hermes/ to keep this PR scoped to the Sentry issues
(refer to the '.hermes/' entry in .gitignore to locate the change).
In `@app/instrumentation-client.ts`:
- Line 47: The new ignoreErrors entries are too broad: remove the raw "invalid
origin" and the loose "/feature named .* was not found/" pattern from the global
ignoreErrors list and instead call the contextual helpers (e.g.,
isInvalidOriginError()) from beforeSend so the error payload, breadcrumbs, and
origin can be inspected first; replace any remaining needed string matches with
precise regex anchored patterns that validate DuckDuckGo’s exact message and
tighten URL/domain filters to strict domain regexes; update the ignore logic in
the instrumentation config and ensure beforeSend invokes isInvalidOriginError()
and any new helper (e.g., isFeatureNotFoundError(payload)) before suppressing
events.
- Around line 1066-1070: The current isWalletErrorMessage branch treats any
"Cannot redefine property" message as a wallet-extension error without evidence;
update the logic in the isWalletErrorMessage check (and the similar checks
between the blocks around lines 1072-1101) to require wallet-specific proof
before returning true: only treat "Cannot redefine property" as a wallet error
when the error message also mentions known wallet-injected property names (e.g.,
injectedProvider, ethereum, web3, scatter) or when the error.stack contains
wallet-specific frames/URLs (e.g., chrome-extension://, moz-extension://, or
known wallet hostnames) matched via a strict regex; otherwise fall back to not
filtering the error so real app TypeErrors surface. Ensure you reference and
update the same predicate used to decide wallet noise so both occurrences behave
identically.
- Around line 2374-2426: The current isMinifiedThreeJsError heuristic is too
broad and can hide unrelated /travel and /combat crashes; update
isMinifiedThreeJsError to require explicit Three/WebGL indicators instead of
generic patterns: tighten the message regex to only match known Three.js
minified error tokens if available, remove or narrow filename.includes("chunk")
and filename.includes("Map") checks, and instead require stack frames that
explicitly reference "three", "three.min", "webgl", "GL", "Renderer", or known
Three.js bundle names, or detect breadcrumbs with
"webglcontextlost"/"contextlost" or an event.handled === true marker; also
replace simple url.includes("/combat"/"/travel") with a stricter URL pattern
(e.g., regex validating path prefix or hostname) so only genuine 3D pages are
matched; update references in the isMinifiedThreeJsError function, specifically
the isMinifiedIdentifier, isFromThreeJs, isMobileBrowser, and isOn3DPage checks
to implement these stricter conditions.
In `@app/src/libs/threejs/util.ts`:
- Around line 596-604: The current code calls isRendererContextValid(renderer)
immediately after new WebGLRenderer(), which treats a detached canvas as invalid
and disposes healthy renderers; instead, defer connectivity validation until the
renderer's DOM element is attached to the document (e.g. check
renderer.domElement.isConnected or run isRendererContextValid only after the
canvas is mounted) and only call renderer.dispose() when isRendererContextValid
still fails after attachment; update the block around WebGLRenderer creation to
skip early validation and perform the isRendererContextValid check post-mount so
healthy renderers aren't prematurely destroyed.
In `@app/src/server/api/routers/combat.ts`:
- Around line 755-758: The catch block inside performAction() currently swallows
all errors and returns a notification string, preventing the outer retry loop
from refetching/replaying version races; change the catch to not return that
notification—either rethrow the caught error (or only transform/return for truly
terminal, non-retryable errors) so the outer retry loop can handle
optimistic-lock desyncs and other transient write errors; locate the try/catch
in performAction() (the block that currently does `catch (_e) { return {
notification: ... } }`) and replace it with logic that preserves/throws the
original error for the caller to handle.
In `@app/src/server/api/routers/staff.ts`:
- Around line 1043-1076: The exponential backoff in deleteUser (and its use of
MAX_RETRIES, BASE_DELAY_MS and MAX_JITTER_MS) can exceed the intended 20–30s
budget (totaling ~153s); cap the retry policy by either reducing MAX_RETRIES or
enforcing a max per-attempt or cumulative delay. Update deleteUser to compute
delayMs = min(BASE_DELAY_MS * 2 ** (attempt - 1) + jitter, MAX_BACKOFF_MS) or
stop retrying when cumulativeDelay + nextDelay > MAX_TOTAL_RETRY_MS (define
MAX_BACKOFF_MS or MAX_TOTAL_RETRY_MS ≈ 20_000–30_000ms), and ensure
Sentry.addBreadcrumb still records the bounded delay and final abort when cap is
reached; adjust constants (MAX_RETRIES, MAX_BACKOFF_MS/MAX_TOTAL_RETRY_MS)
accordingly so deleteUser cannot sleep past request timeouts.
In `@app/src/server/api/routers/travel.ts`:
- Around line 233-250: The current Promise.all ties initiateBattle to
breakStealth so a failure in breakStealth can hide a successful battle; instead
call initiateBattle(...) first and await its result to obtain the battle (use
the existing initiateBattle symbol and the same payload), then run
breakStealth(ctx.drizzle, ctx.userId, user.stealth, false) as a best-effort
cleanup (only if user.stealthActive) without blocking or throwing back to the
caller — e.g., trigger it in a detached promise and handle/log errors locally so
the battleId is always returned to the client.
---
Outside diff comments:
In `@app/src/layout/Sector.tsx`:
- Around line 596-629: The effect currently gates movement on userData.avatar so
IMG_AVATAR_DEFAULT never gets used; remove the strict avatar requirement in the
initial guard so the effect runs when userData exists (keep checking
userData.status inside the effect), i.e. change the condition from "... &&
userData && userData.avatar" to "... && userData" in the useEffect, leaving the
move(...) call as-is so avatar and avatarLight fallback to IMG_AVATAR_DEFAULT
when userData.avatar is missing.
In `@app/src/server/api/routers/staff.ts`:
- Around line 1192-1207: Batch 10 deletes mpvpBattleUser rows but doesn't clean
up related mpvpBattleQueue state, leaving empty teams or stuck "claiming-*"
battle IDs; update the deletion block to also prune mpvpBattleQueue entries
related to the removed user by: remove queue rows where the team owner or any
team member equals userId (reference mpvpBattleQueue and mpvpBattleUser), delete
or clear mpvpBattleQueue rows whose battleId matches the "claiming-%" pattern
for that user's teams, and ensure the same repair logic used in forceAwake is
applied (or call the same helper) so empty teams and claiming-* IDs are cleared
when deleting a queued user.
In `@app/src/server/api/routers/travel.ts`:
- Around line 160-203: The current Promise.all groups the two money updates with
side effects (breakStealth and clan update) so a failure in one can leave
partial commits; change the flow in the rob handler to first run only the two
money updates via Promise.all on the two ctx.drizzle.update(...) calls that
produce robberUpdate and targetUpdate, check robberUpdate.rowsAffected and
targetUpdate.rowsAffected and perform the rollback (using the existing
ctx.drizzle.update that reverses robber changes) if either fails, and only after
both money updates succeed run the side effects (call breakStealth(ctx.drizzle,
ctx.userId, user.stealth, false) and the ctx.drizzle.update(...) for clan
points) separately and handle their failures independently so they cannot cause
money updates to partially commit.
---
Nitpick comments:
In `@app/src/libs/threejs/util.ts`:
- Around line 905-915: The shader creation probe in isWebGLContextValid() is
being executed every animation frame (via isRendererContextValid(renderer)),
which is expensive; move that expensive probe out of the per-frame path by
removing createShader/deleteShader from isWebGLContextValid() and instead
perform the shader probe only during context initialization or on demand (e.g.,
a new probe function like probeShaderSupportOnce or a memoized check with a
timestamp/flag), and update isRendererContextValid(renderer) to rely on the
lightweight checks in isWebGLContextValid() and the cached probe result or
event-driven re-check triggered on context lost/restored; ensure you reference
and update the functions isWebGLContextValid() and
isRendererContextValid(renderer) and any new probe function/flag used for
caching.
In `@app/src/server/api/routers/staff.ts`:
- Around line 484-489: The destructure currently awaits four parallel calls but
the third result (_userAttributes) is never used; remove the unnecessary
fetchAttributes(ctx.drizzle, ctx.userId) call and adjust the Promise.all to only
run the needed three calls (fetchUser(ctx.drizzle, ctx.userId),
fetchUser(ctx.drizzle, input.userId), fetchAttributes(ctx.drizzle,
input.userId)), updating the destructured variables accordingly so
_userAttributes is no longer created.
🪄 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: a9583531-2f9c-4747-9a79-293a5a29afbc
⛔ Files ignored due to path filters (1)
app/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.gitignoreapp/instrumentation-client.tsapp/package.jsonapp/src/app/_trpc/Provider.tsxapp/src/app/travel/page.tsxapp/src/layout/Map.tsxapp/src/layout/Sector.tsxapp/src/layout/StatusBar.tsxapp/src/layout/StrengthWeaknesses.tsxapp/src/libs/spacetimedb/client.tsapp/src/libs/threejs/shaders.tsapp/src/libs/threejs/util.tsapp/src/server/api/routers/combat.tsapp/src/server/api/routers/staff.tsapp/src/server/api/routers/travel.ts
MathiasGruber
force-pushed
the
fix/sentry-batch-theninjarpg_0f69c0
branch
from
6234da1 to
b389362
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/src/server/api/routers/staff.ts (1)
484-489: Remove unused database fetch.
_userAttributesis fetched but never used, adding an unnecessary database round-trip. If this is preparation for future functionality, defer adding the query until it's actually needed.♻️ Proposed fix
- const [user, target, _userAttributes, targetAttributes] = await Promise.all([ + const [user, target, targetAttributes] = await Promise.all([ fetchUser(ctx.drizzle, ctx.userId), fetchUser(ctx.drizzle, input.userId), - fetchAttributes(ctx.drizzle, ctx.userId), fetchAttributes(ctx.drizzle, input.userId), ]);Based on learnings: "CRITICAL: Minimize database round-trips in tRPC endpoints."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/server/api/routers/staff.ts` around lines 484 - 489, The code is making an unnecessary DB call by fetching _userAttributes via fetchAttributes in the Promise.all even though _userAttributes is never used; remove that unused fetch from the Promise.all and update the destructuring to only capture the needed results (e.g., const [user, target, targetAttributes] = await Promise.all([...]) or split into two awaits if clearer), ensuring any references to _userAttributes are removed and no other logic expects that value; specifically edit the Promise.all invocation that calls fetchAttributes(ctx.drizzle, ctx.userId) and the corresponding destructured variable name.app/src/libs/threejs/shaders.ts (1)
177-200: Consider extracting shared WebGL context validation.The context validation logic (lines 177-200) is duplicated from
applyWindShader(lines 37-60). Consider extracting to a shared helper:♻️ Optional: Extract shared validation helper
// At top of file or in util.ts const isWebGLContextReady = (renderer: WebGLRenderer): boolean => { try { const gl = renderer.getContext(); if (!gl || gl.isContextLost()) { console.warn("Skipping shader compilation: WebGL context is lost"); return false; } const testShader = gl.createShader(gl.VERTEX_SHADER); if (!testShader) { console.warn("Skipping shader compilation: Cannot create shaders (context invalid)"); return false; } gl.deleteShader(testShader); return true; } catch (e) { console.warn("Skipping shader compilation due to context error:", e); return false; } };Then use in both functions:
material.onBeforeCompile = (shader, renderer) => { if (!isWebGLContextReady(renderer)) return; // ... rest of shader setup };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/libs/threejs/shaders.ts` around lines 177 - 200, Extract the duplicated WebGL context checks into a shared helper (e.g., isWebGLContextReady) and replace the inline try/catch block inside material.onBeforeCompile and the same block in applyWindShader with a single call to that helper; the helper should accept the renderer, return a boolean, perform the getContext/isContextLost and createShader/deleteShader checks, and log the same warning messages so both material.onBeforeCompile and applyWindShader simply return early when the helper returns false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/instrumentation-client.ts`:
- Around line 2401-2405: The current isOn3DPage check using
url.includes("/combat") || url.includes("/travel") is too broad and matches
unintended paths; update the check to parse the URL and test the pathname
segments precisely (for example using the URL API or regex that enforces segment
boundaries such as testing pathname === "/combat" or
pathname.startsWith("/combat/") and the same for "/travel") so the logic in
isOn3DPage only returns true for the intended /combat and /travel routes; update
references to the url variable and the isOn3DPage boolean accordingly.
---
Nitpick comments:
In `@app/src/libs/threejs/shaders.ts`:
- Around line 177-200: Extract the duplicated WebGL context checks into a shared
helper (e.g., isWebGLContextReady) and replace the inline try/catch block inside
material.onBeforeCompile and the same block in applyWindShader with a single
call to that helper; the helper should accept the renderer, return a boolean,
perform the getContext/isContextLost and createShader/deleteShader checks, and
log the same warning messages so both material.onBeforeCompile and
applyWindShader simply return early when the helper returns false.
In `@app/src/server/api/routers/staff.ts`:
- Around line 484-489: The code is making an unnecessary DB call by fetching
_userAttributes via fetchAttributes in the Promise.all even though
_userAttributes is never used; remove that unused fetch from the Promise.all and
update the destructuring to only capture the needed results (e.g., const [user,
target, targetAttributes] = await Promise.all([...]) or split into two awaits if
clearer), ensuring any references to _userAttributes are removed and no other
logic expects that value; specifically edit the Promise.all invocation that
calls fetchAttributes(ctx.drizzle, ctx.userId) and the corresponding
destructured variable name.
🪄 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: 9ac5e281-f85b-40c5-a7f4-cc6d5609994f
⛔ Files ignored due to path filters (1)
app/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.gitignore.hermes/worktrees/7919cdbe_feature-fix_all_sentry_issuesapp/instrumentation-client.tsapp/package.jsonapp/src/app/_trpc/Provider.tsxapp/src/app/travel/page.tsxapp/src/layout/Map.tsxapp/src/layout/Sector.tsxapp/src/layout/StatusBar.tsxapp/src/layout/StrengthWeaknesses.tsxapp/src/libs/spacetimedb/client.tsapp/src/libs/threejs/shaders.tsapp/src/libs/threejs/util.tsapp/src/server/api/routers/combat.tsapp/src/server/api/routers/staff.tsapp/src/server/api/routers/travel.ts
💤 Files with no reviewable changes (1)
- .gitignore
✅ Files skipped from review due to trivial changes (1)
- .hermes/worktrees/7919cdbe_feature-fix_all_sentry_issues
🚧 Files skipped from review as they are similar to previous changes (9)
- app/src/app/_trpc/Provider.tsx
- app/src/layout/StrengthWeaknesses.tsx
- app/src/layout/Map.tsx
- app/src/layout/Sector.tsx
- app/src/app/travel/page.tsx
- app/src/libs/spacetimedb/client.ts
- app/src/server/api/routers/travel.ts
- app/src/server/api/routers/combat.ts
- app/src/libs/threejs/util.ts
MathiasGruber
force-pushed
the
fix/sentry-batch-theninjarpg_0f69c0
branch
from
b389362 to
a026742
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/server/api/routers/travel.ts (1)
161-203:⚠️ Potential issue | 🔴 CriticalDecouple non-critical side effects from money transfer and guard target debit atomically.
Right now
breakStealth/clan updates are part of the samePromise.allas the money writes. If either side-effect throws, the mutation can fail after balances were already changed. Also, the target debit lacks amoney >= stolenAmountguard, so concurrent balance changes can produce invalid transfer outcomes.Suggested fix
- const [robberUpdate, targetUpdate] = await Promise.all([ + const [robberUpdate, targetUpdate] = await Promise.all([ ctx.drizzle .update(userData) .set({ money: sql`${userData.money} + ${stolenAmount}`, villagePrestige: sql`${userData.villagePrestige} + ${ROBBING_VILLAGE_PRESTIGE_GAIN}`, }) .where(eq(userData.userId, ctx.userId)), ctx.drizzle .update(userData) .set({ money: sql`${userData.money} - ${stolenAmount}`, robImmunityUntil: secondsFromNow(ROBBING_IMMUNITY_DURATION), }) - .where(eq(userData.userId, input.userId)), - ...(user.stealthActive - ? [breakStealth(ctx.drizzle, ctx.userId, user.stealth, false)] - : []), - ...(user.clanId - ? [ - ctx.drizzle - .update(clan) - .set({ - points: sql`${clan.points} + 1`, - activityPoints: sql`${clan.activityPoints} + 1`, - }) - .where(eq(clan.id, user.clanId)), - ] - : []), + .where( + and( + eq(userData.userId, input.userId), + gte(userData.money, stolenAmount), + ), + ), ]); if (robberUpdate.rowsAffected === 0) { return errorResponse("Failed to update robber's data"); } if (targetUpdate.rowsAffected === 0) { @@ return errorResponse("Failed to update target's data"); } + + await Promise.allSettled([ + ...(user.stealthActive + ? [breakStealth(ctx.drizzle, ctx.userId, user.stealth, false)] + : []), + ...(user.clanId + ? [ + ctx.drizzle + .update(clan) + .set({ + points: sql`${clan.points} + 1`, + activityPoints: sql`${clan.activityPoints} + 1`, + }) + .where(eq(clan.id, user.clanId)), + ] + : []), + ]);Based on learnings: "NEVER use database transactions - PlanetScale does not support traditional transactions. Instead, use guard clauses with WHERE conditions to ensure atomic updates (e.g.,
WHERE balance >= amountto prevent negative balances)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/server/api/routers/travel.ts` around lines 161 - 203, Make the money transfer atomic by separating the balance updates from non-critical side effects: run only the two money updates in Promise.all — the robber update (ctx.drizzle.update(userData) setting money += stolenAmount and villagePrestige) and the target update (ctx.drizzle.update(userData) setting money -= stolenAmount) — but change the target update to include a guard WHERE money >= stolenAmount (e.g., .where(eq(userData.userId, input.userId), sql`${userData.money} >= ${stolenAmount}`)) so the DB will refuse the debit if funds are insufficient; check rowsAffected for both updates and return errors / rollback the robber update if the target update failed. Remove breakStealth(ctx.drizzle, ctx.userId, user.stealth, false) and the clan update from that Promise.all and instead trigger them after the money transfer succeeds (run them separately and handle/log any errors without undoing the completed transfer).
♻️ Duplicate comments (1)
app/src/server/api/routers/travel.ts (1)
233-238:⚠️ Potential issue | 🟠 MajorDon’t let stealth cleanup failure block battle creation.
If
breakStealth(...)throws, combat never starts and the rob-fail flow aborts. Handle stealth-break errors locally so battle initiation still runs.Suggested fix
if (user.stealthActive) { - await breakStealth(ctx.drizzle, ctx.userId, user.stealth, false); + await breakStealth(ctx.drizzle, ctx.userId, user.stealth, false).catch( + (error) => { + console.error("Failed to break stealth before rob combat", error); + }, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/server/api/routers/travel.ts` around lines 233 - 238, The breakStealth(...) call can throw and currently aborts the rob-fail flow, so wrap the call to breakStealth(ctx.drizzle, ctx.userId, user.stealth, false) in a local try/catch inside travel.ts (around the existing if (user.stealthActive) block), log or swallow the error (e.g., processLogger.warn or ctx.logger.warn) and continue execution so that the subsequent battle creation/initiating code (the initiateBattle or equivalent combat-start logic) always runs even if stealth cleanup fails.
🧹 Nitpick comments (1)
app/src/server/api/routers/travel.ts (1)
515-516: Move the new avatar Zod preprocessors into/app/src/validators/.These new schema rules are added inline in a router; extract them into a validator module and import them here to match project schema placement conventions.
As per coding guidelines: "All Zod schemas should be defined in
/app/src/validators/directory, not in page components or routers. Import schemas from this directory rather than defining them inline".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/server/api/routers/travel.ts` around lines 515 - 516, The avatar preprocessors (avatar and avatarLight using IMG_AVATAR_DEFAULT) are defined inline in the travel router; extract them into a new validator module under the validators directory (e.g., export const avatarSchema = z.preprocess(...).refine/ z.string().url() and avatarLightSchema similarly, referencing IMG_AVATAR_DEFAULT from the same module or a shared constants module), update the router to import avatarSchema and avatarLightSchema and replace the inline definitions with those imports, and ensure the new validator module is exported for reuse and types remain compatible with the existing schema usage in the router.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/app/_trpc/Provider.tsx`:
- Around line 152-156: The 429 JSON-parse detector (isJsonParseError in
Provider.tsx) is missing the Safari and '"... is not valid JSON"' variants
included in the 403 matcher; update the isJsonParseError condition used in the
429 handler to mirror the 403 block by adding the same includes checks (e.g.,
add the '"is not valid JSON"' and the Safari-specific message checks used by the
403 matcher) so the same clients fall into the non-destructive path when JSON
parsing fails for 429 responses.
- Around line 126-141: The current early return swallows all 403+invalid-JSON
tRPC errors; tighten it by detecting the Clerk/session-expiry signature instead
of returning on any retryable 403. Update the condition around TRPCClientError,
(error.data as { httpStatus?: number })?.httpStatus === 403, and
isRetryableTrpcError(error) to also check for an auth-expiry marker (for example
inspect error.data.raw or error.data.body for Clerk sign-in HTML, presence of
"/sign-in" or "Sign in" text, or a Clerk-specific header), and if detected call
the existing soft sign-in/session-expired flow (e.g., invoke the same handler
used by useRequiredUserData or a sessionExpired handler) rather than silently
returning; otherwise let the error proceed to console.error/Sentry so real
access/outage issues are surfaced.
In `@app/src/libs/spacetimedb/client.ts`:
- Around line 142-143: The comment and wsReady logic are misleading: wsReady is
set true immediately after calling setupGlobalSubscriptions(), but subscribe()
is asynchronous and onApplied fires only after the server's SubscribeApplied
round-trip; update the code so wsReady becomes true only after subscriptions are
actually applied (e.g., track subscribe() callbacks/onApplied for each
subscription and await them before setting wsReady), or change the comment to
explicitly state wsReady means "subscriptions sent" not "applied"; also ensure
onConnect behavior is clarified or guarded by checking readyState === OPEN (and
the IdentityToken receipt) before treating the socket as fully open; adjust
references in setupGlobalSubscriptions(), subscribe(), onApplied, wsReady,
onConnect, and IdentityToken handling accordingly.
---
Outside diff comments:
In `@app/src/server/api/routers/travel.ts`:
- Around line 161-203: Make the money transfer atomic by separating the balance
updates from non-critical side effects: run only the two money updates in
Promise.all — the robber update (ctx.drizzle.update(userData) setting money +=
stolenAmount and villagePrestige) and the target update
(ctx.drizzle.update(userData) setting money -= stolenAmount) — but change the
target update to include a guard WHERE money >= stolenAmount (e.g.,
.where(eq(userData.userId, input.userId), sql`${userData.money} >=
${stolenAmount}`)) so the DB will refuse the debit if funds are insufficient;
check rowsAffected for both updates and return errors / rollback the robber
update if the target update failed. Remove breakStealth(ctx.drizzle, ctx.userId,
user.stealth, false) and the clan update from that Promise.all and instead
trigger them after the money transfer succeeds (run them separately and
handle/log any errors without undoing the completed transfer).
---
Duplicate comments:
In `@app/src/server/api/routers/travel.ts`:
- Around line 233-238: The breakStealth(...) call can throw and currently aborts
the rob-fail flow, so wrap the call to breakStealth(ctx.drizzle, ctx.userId,
user.stealth, false) in a local try/catch inside travel.ts (around the existing
if (user.stealthActive) block), log or swallow the error (e.g.,
processLogger.warn or ctx.logger.warn) and continue execution so that the
subsequent battle creation/initiating code (the initiateBattle or equivalent
combat-start logic) always runs even if stealth cleanup fails.
---
Nitpick comments:
In `@app/src/server/api/routers/travel.ts`:
- Around line 515-516: The avatar preprocessors (avatar and avatarLight using
IMG_AVATAR_DEFAULT) are defined inline in the travel router; extract them into a
new validator module under the validators directory (e.g., export const
avatarSchema = z.preprocess(...).refine/ z.string().url() and avatarLightSchema
similarly, referencing IMG_AVATAR_DEFAULT from the same module or a shared
constants module), update the router to import avatarSchema and
avatarLightSchema and replace the inline definitions with those imports, and
ensure the new validator module is exported for reuse and types remain
compatible with the existing schema usage in the router.
🪄 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: 7efb2840-8d43-4777-940e-d4fede414e90
📒 Files selected for processing (6)
.gitignoreapp/instrumentation-client.tsapp/src/app/_trpc/Provider.tsxapp/src/layout/StatusBar.tsxapp/src/libs/spacetimedb/client.tsapp/src/server/api/routers/travel.ts
💤 Files with no reviewable changes (1)
- .gitignore
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/libs/spacetimedb/client.ts (1)
142-149:⚠️ Potential issue | 🔴 CriticalSurface initial subscription failures to
connect()callers.
waitForConnection()blocks on bothconnectionState === "connected"ANDwsReady === true(line 148), but theonErrorhandler for global subscription (lines 303–306) never transitions the state machine. If the subscription fails beforeonApplied,wsReadystays false andconnectionStateremains "connected", causingwaitForConnection()to hang indefinitely instead of rejecting.The same pattern exists for guest session (lines 339–342) and session subscriptions (lines 377–380).
Suggested fix
.onError((ctx: ErrorContext) => { console.error("[SpacetimeDB] Global subscription error:", ctx); + this.wsReady = false; + this.connectionState = "error"; + this.emit({ type: "connection_state", state: "error" }); this.emit({ type: "error", message: "Global subscription error" }); })Apply the same fix to guest session and session subscription error handlers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/libs/spacetimedb/client.ts` around lines 142 - 149, waitForConnection() can hang because subscription onError handlers never update the connection state or reject the pending connect promise; update the global subscription onError (and mirror the same change in the guest session and session subscription onError handlers) to transition the connection state to a failure state and propagate the error to connect callers—e.g., set this.connectionState = "failed" (or call an existing transition method) and surface the error by rejecting the pending connect/wait promise or invoking the stored reject callback so waitForConnection() does not block indefinitely when the subscription fails before onApplied.
♻️ Duplicate comments (1)
app/src/libs/spacetimedb/client.ts (1)
277-281:⚠️ Potential issue | 🟠 MajorGuest handoff leaves stale subscription state behind.
This branch returns with any previous authenticated
globalSubscriptionstill alive andwsReadystilltrue. On an authenticated → guest handoff, the old stream can stay attached to this singleton, andcreateSession()cannot distinguish “guest connected” from “guest seed subscription established” becausesubscribeToGuestSession()never re-armswsReady.Suggested fix
if (this.currentUserId === "guest") { + if (this.globalSubscription) { + this.globalSubscription.unsubscribe(); + this.globalSubscription = null; + } this.setupTableHandlers(); // Mark WebSocket as ready immediately for guests (no subscription to wait for) this.wsReady = true; return; }subscribeToGuestSession(seed: string): void { if (!this.connection) return; + this.wsReady = false; // Unsubscribe from previous global subscription if any if (this.globalSubscription) { this.globalSubscription.unsubscribe(); this.globalSubscription = null; @@ .onApplied(() => { if (DEBUG) { console.log( "[SpacetimeDB] Guest session subscription applied for seed:", seed, ); } + this.wsReady = true; })In the current SpacetimeDB TypeScript SDK, does `subscriptionBuilder().subscribe(...)` return before `onApplied` fires, with `onApplied` only running after the server confirms the subscription has been applied?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/libs/spacetimedb/client.ts` around lines 277 - 281, When handling an authenticated→guest handoff in the branch where this.currentUserId === "guest" inside setupTableHandlers(), ensure any previous authenticated globalSubscription is cleaned up and wsReady is not left true prematurely: explicitly cancel or unsubscribe the existing globalSubscription (reference globalSubscription) and set wsReady = false before calling setupTableHandlers()/subscribeToGuestSession(), then have subscribeToGuestSession() re-arm wsReady only when its subscription confirm callback (e.g., onApplied or equivalent) fires; also ensure createSession() relies on that onApplied-driven wsReady so it can distinguish “guest connected” from “guest seed subscription established.”
🧹 Nitpick comments (1)
app/src/server/api/routers/travel.ts (1)
160-214: Rollback logic is best-effort, not atomic.The manual rollback on lines 197-203 and 207-213 could itself fail silently if the database is under load. Since PlanetScale doesn't support transactions, this is acceptable but worth noting:
- If the rollback fails, money could be in an inconsistent state
- Consider logging rollback failures to Sentry for monitoring
The parallel update pattern with
Promise.allis correct per guidelines. The conditional spreads for stealth/clan updates are well-structured.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/server/api/routers/travel.ts` around lines 160 - 214, Rollback operations after the Promise.all updates are best-effort and may fail silently; wrap each manual rollback (the ctx.drizzle.update calls that restore money/villagePrestige for userData when robberUpdate.rowsAffected or targetUpdate.rowsAffected is 0) in try/catch and report any caught errors to your monitoring (e.g., call Sentry.captureException(error, { extra: { ctxUserId: ctx.userId, inputUserId: input.userId, stolenAmount } }) or processLogger.error with the same context) so rollback failures are visible; keep the current errorResponse returns but ensure the catch does not swallow the monitoring call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/instrumentation-client.ts`:
- Around line 2386-2463: The bug is that isOn3DPage is block-scoped inside the
try/catch and referenced later in isMinifiedThreeJsError, causing a
ReferenceError; fix it by declaring let isOn3DPage = false; before the try block
and then assign to isOn3DPage inside the try (setting based on urlObj.pathname)
and inside the catch (setting based on url.includes), so the final desktop check
(if (!isMobileBrowser && isBrowserLevelError && isOn3DPage)) can safely use it;
update references to the variable name exactly as used in
isMinifiedThreeJsError.
- Around line 2327-2359: The isWebGLShaderContextLossError function incorrectly
checks a custom tag (event.tags?.handled === "yes") that isn't used; update it
to read the standard Sentry mechanism handled flag on the exception value
instead (use event.exception?.values?.[0]?.mechanism?.handled === true) so
isHandled is derived from the exception mechanism; modify the
isWebGLShaderContextLossError implementation to replace the tags check with this
mechanism-based boolean.
---
Outside diff comments:
In `@app/src/libs/spacetimedb/client.ts`:
- Around line 142-149: waitForConnection() can hang because subscription onError
handlers never update the connection state or reject the pending connect
promise; update the global subscription onError (and mirror the same change in
the guest session and session subscription onError handlers) to transition the
connection state to a failure state and propagate the error to connect
callers—e.g., set this.connectionState = "failed" (or call an existing
transition method) and surface the error by rejecting the pending connect/wait
promise or invoking the stored reject callback so waitForConnection() does not
block indefinitely when the subscription fails before onApplied.
---
Duplicate comments:
In `@app/src/libs/spacetimedb/client.ts`:
- Around line 277-281: When handling an authenticated→guest handoff in the
branch where this.currentUserId === "guest" inside setupTableHandlers(), ensure
any previous authenticated globalSubscription is cleaned up and wsReady is not
left true prematurely: explicitly cancel or unsubscribe the existing
globalSubscription (reference globalSubscription) and set wsReady = false before
calling setupTableHandlers()/subscribeToGuestSession(), then have
subscribeToGuestSession() re-arm wsReady only when its subscription confirm
callback (e.g., onApplied or equivalent) fires; also ensure createSession()
relies on that onApplied-driven wsReady so it can distinguish “guest connected”
from “guest seed subscription established.”
---
Nitpick comments:
In `@app/src/server/api/routers/travel.ts`:
- Around line 160-214: Rollback operations after the Promise.all updates are
best-effort and may fail silently; wrap each manual rollback (the
ctx.drizzle.update calls that restore money/villagePrestige for userData when
robberUpdate.rowsAffected or targetUpdate.rowsAffected is 0) in try/catch and
report any caught errors to your monitoring (e.g., call
Sentry.captureException(error, { extra: { ctxUserId: ctx.userId, inputUserId:
input.userId, stolenAmount } }) or processLogger.error with the same context) so
rollback failures are visible; keep the current errorResponse returns but ensure
the catch does not swallow the monitoring call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b9aeb43-fd1a-4e9b-a77e-03b8f4d84551
📒 Files selected for processing (9)
app/instrumentation-client.tsapp/src/app/_trpc/Provider.tsxapp/src/layout/Map.tsxapp/src/layout/Sector.tsxapp/src/libs/spacetimedb/client.tsapp/src/libs/threejs/util.tsapp/src/server/api/routers/combat.tsapp/src/server/api/routers/staff.tsapp/src/server/api/routers/travel.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/server/api/routers/staff.ts
- app/src/server/api/routers/combat.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/server/api/routers/travel.ts (1)
176-216:⚠️ Potential issue | 🟡 MinorRollback logic does not restore stealth or clan points.
If
robberUpdateortargetUpdatefails, the rollback only reverses the money/prestige changes but leavesbreakStealthand clan point increments applied. This creates partial state inconsistency.Additionally, the heterogeneous
Promise.all(mixing drizzle results withbreakStealth's boolean return) makes index-based access fragile.Consider either:
- Separating the critical updates (robber/target money) from best-effort side effects (stealth, clan points)
- Adding rollback for stealth reactivation and clan point decrement
💡 Suggested approach: separate critical vs best-effort operations
- // Update robber's money, target's money, break stealth, and update clan points in parallel - const results = await Promise.all([ - ctx.drizzle - .update(userData) - .set({ - money: sql`${userData.money} + ${stolenAmount}`, - villagePrestige: sql`${userData.villagePrestige} + ${ROBBING_VILLAGE_PRESTIGE_GAIN}`, - }) - .where(eq(userData.userId, ctx.userId)), + // Critical updates: robber and target money changes + const [robberUpdate, targetUpdate] = await Promise.all([ ctx.drizzle .update(userData) .set({ - money: sql`${userData.money} - ${stolenAmount}`, - robImmunityUntil: secondsFromNow(ROBBING_IMMUNITY_DURATION), + money: sql`${userData.money} + ${stolenAmount}`, + villagePrestige: sql`${userData.villagePrestige} + ${ROBBING_VILLAGE_PRESTIGE_GAIN}`, }) - .where(eq(userData.userId, input.userId)), - ...(user.stealthActive - ? [breakStealth(ctx.drizzle, ctx.userId, user.stealth, false)] - : []), - ...(user.clanId - ? [ - ctx.drizzle - .update(clan) - .set({ - points: sql`${clan.points} + 1`, - activityPoints: sql`${clan.activityPoints} + 1`, - }) - .where(eq(clan.id, user.clanId)), - ] - : []), + .where(eq(userData.userId, ctx.userId)), + ctx.drizzle + .update(userData) + .set({ + money: sql`${userData.money} - ${stolenAmount}`, + robImmunityUntil: secondsFromNow(ROBBING_IMMUNITY_DURATION), + }) + .where(eq(userData.userId, input.userId)), ]); - const robberUpdate = results[0]; - const targetUpdate = results[1]; - if (robberUpdate.rowsAffected === 0) { // Rollback target update if robber update fails (restore money and remove immunity) await ctx.drizzle @@ ... @@ return errorResponse("Failed to update target's data"); } + + // Best-effort side effects (non-critical, no rollback needed) + await Promise.all([ + ...(user.stealthActive + ? [breakStealth(ctx.drizzle, ctx.userId, user.stealth, false)] + : []), + ...(user.clanId + ? [ + ctx.drizzle + .update(clan) + .set({ + points: sql`${clan.points} + 1`, + activityPoints: sql`${clan.activityPoints} + 1`, + }) + .where(eq(clan.id, user.clanId)), + ] + : []), + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/server/api/routers/travel.ts` around lines 176 - 216, The rollback currently restores only money/prestige but not the side-effects from breakStealth(...) and the clan points update, and mixing breakStealth's boolean with drizzle results makes index-based checks fragile; refactor so that the critical updates (the two ctx.drizzle.update(userData) operations that produce robberUpdate and targetUpdate) run together and are checked/rolled back first, then run best-effort side-effects separately (call breakStealth(ctx.drizzle, ctx.userId, ...) and ctx.drizzle.update(clan) after confirming both critical updates succeeded), or if you prefer to keep them together add compensating rollbacks: if robberUpdate fails undo breakStealth (reactivate stealth) and if either update fails decrement clan points via ctx.drizzle.update(clan) to reverse the earlier increment; reference functions and symbols: breakStealth, ctx.drizzle.update(clan), robberUpdate, targetUpdate, userData, ROBBING_VILLAGE_PRESTIGE_GAIN when implementing the separation or compensating updates.app/src/libs/spacetimedb/client.ts (1)
306-309:⚠️ Potential issue | 🟠 MajorFail fast on subscription failure to avoid hanging
connect().
waitForConnection()only rejects when state becomes"error"/"disconnected". On Line 306-309,onErroremits an error event but leavesconnectionStateas"connecting", soconnect()can hang indefinitely if subscription application fails without a disconnect.🔧 Proposed fix
.onError((ctx: ErrorContext) => { console.error("[SpacetimeDB] Global subscription error:", ctx); + this.wsReady = false; + this.connectionState = "error"; + this.emit({ type: "connection_state", state: "error" }); this.emit({ type: "error", message: "Global subscription error" }); })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/libs/spacetimedb/client.ts` around lines 306 - 309, The onError handler currently only emits an error event so waitForConnection/connect can hang; update the .onError((ctx: ErrorContext) => { ... }) handler to both include the actual error (ctx) in the emitted error event and transition the connection state to an error state so waitForConnection rejects—e.g., emit the error with ctx and set the internal connectionState to "error" (or call the existing setConnectionState/setState helper) so connect()/waitForConnection will fail fast when subscription application fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/instrumentation-client.ts`:
- Around line 979-985: The breadcrumb 429 check in the has429Status computation
only matches numeric 429 and will miss string "429"; update the predicate used
in has429Status (the event.breadcrumbs?.some callback that checks b.category ===
"fetch" and b.data?.status_code) to normalize the status (e.g., coerce with
Number(...) and compare to 429, or compare both 429 and "429") so both numeric
and string status values are detected consistently with other checks in this
file.
---
Outside diff comments:
In `@app/src/libs/spacetimedb/client.ts`:
- Around line 306-309: The onError handler currently only emits an error event
so waitForConnection/connect can hang; update the .onError((ctx: ErrorContext)
=> { ... }) handler to both include the actual error (ctx) in the emitted error
event and transition the connection state to an error state so waitForConnection
rejects—e.g., emit the error with ctx and set the internal connectionState to
"error" (or call the existing setConnectionState/setState helper) so
connect()/waitForConnection will fail fast when subscription application fails.
In `@app/src/server/api/routers/travel.ts`:
- Around line 176-216: The rollback currently restores only money/prestige but
not the side-effects from breakStealth(...) and the clan points update, and
mixing breakStealth's boolean with drizzle results makes index-based checks
fragile; refactor so that the critical updates (the two
ctx.drizzle.update(userData) operations that produce robberUpdate and
targetUpdate) run together and are checked/rolled back first, then run
best-effort side-effects separately (call breakStealth(ctx.drizzle, ctx.userId,
...) and ctx.drizzle.update(clan) after confirming both critical updates
succeeded), or if you prefer to keep them together add compensating rollbacks:
if robberUpdate fails undo breakStealth (reactivate stealth) and if either
update fails decrement clan points via ctx.drizzle.update(clan) to reverse the
earlier increment; reference functions and symbols: breakStealth,
ctx.drizzle.update(clan), robberUpdate, targetUpdate, userData,
ROBBING_VILLAGE_PRESTIGE_GAIN when implementing the separation or compensating
updates.
🪄 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: 933f473d-de03-4fdd-9013-787a2969b1a7
📒 Files selected for processing (4)
app/instrumentation-client.tsapp/src/app/_trpc/Provider.tsxapp/src/libs/spacetimedb/client.tsapp/src/server/api/routers/travel.ts
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
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
Fixed Sentry issues: THENINJARPG-2D1, THENINJARPG-2FQ, THENINJARPG-2JB, THENINJARPG-2CN, THENINJARPG-2JD, THENINJARPG-2J9, THENINJARPG-2JA, THENINJARPG-2FX, THENINJARPG-2J8, THENINJARPG-2JE, THENINJARPG-2J6, THENINJARPG-1NM, THENINJARPG-2G0, THENINJARPG-163, THENINJARPG-2GN, THENINJARPG-2G2, THENINJARPG-2JG, THENINJARPG-2JF, THENINJARPG-2G1**
Context
Automated batch Sentry fix operation
Closes #1233
Closes #1234
Closes #1235
Closes #1236
Closes #1237
Closes #1238
Closes #1239
Closes #1240
Closes #1241
Closes #1242
Closes #1243
Closes #1244
Closes #1245
Closes #1246
Closes #1247
Closes #1248
Closes #1249
Closes #1250
Closes #1251
Note
Medium Risk
Moderate risk: expands Sentry client-side filtering and adjusts several runtime edge-case behaviors (WebGL context validation, rate limiting, subscription readiness, and user deletion retries) that could inadvertently hide actionable errors or change timing-dependent flows.
Overview
Improves client Sentry signal quality by adding new ignore/filter rules for Safari JSON parse failures, rate-limit (429) parse errors, server action errors, wallet-extension noise, and WebGL/Three.js context-loss/minified errors, plus broadens React scheduler filtering to scan all exception values.
Hardens WebGL rendering on
Map/Sectorand shader compilation by validating renderer/context health (isRendererContextValid) and skipping rendering/shader mutation when the context is lost/invalid, reducing iOS/Safari shader failures.Tightens a few runtime correctness issues: guards empty-body POSTs in the MCP route, makes travel sector queries and avatar handling more defensive, avoids NaNs in
StrengthWeaknesses, refines robbery updates to break stealth and sequence/rollback updates more safely, and updatesSpacetimeDBconnect readiness to wait for subscriptions before reporting "connected".Backend ops tweaks: improves battle error logging, fixes a staff clone attribute fetch bug, expands
deleteUserdeadlock retries with exponential backoff+jitter and Sentry breadcrumbs, and ensures more related records are deleted. Also bumps Tailwind dev dependencies to4.2.1(and related lockfile updates).Written by Cursor Bugbot for commit 13588b4. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Bug Fixes
Improvements
Chores