fix: normalize GraphQL not found detail responses#39
Conversation
📝 WalkthroughWalkthroughThe PR normalizes GraphQL "not found" errors across eight detail pages. A shared ChangesGraphQL NOT_FOUND Error Normalization
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
7966791 to
5994e90
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/Locations/model/LocationDetailViewModel.ts`:
- Line 220: The return expression in LocationDetailViewModel uses a redundant
check: remove the unnecessary && !this.showNotFound from the guard (keep
Boolean(this.item) && !this.showError) because showNotFound is already derived
from !this.item; update the return in the getter/method that currently returns
"Boolean(this.item) && !this.showError && !this.showNotFound" to
"Boolean(this.item) && !this.showError" so behavior remains the same but the
condition is simplified.
In `@src/pages/Locations/ui/LocationDetailPage/LocationDetailPage.tsx`:
- Around line 40-46: The not-found branch is using generic error copy
(vm.copy.detail.errorTitle / errorDescription) which implies a failure; update
the not-found rendering to use dedicated not-found copy keys (e.g.,
vm.copy.detail.notFoundTitle and vm.copy.detail.notFoundDescription) in the
renderWhen(vm.showNotFound, <StatePanel ... />) call and update the ViewModel to
supply those keys; also add a safe fallback to the existing
errorTitle/errorDescription if the new notFound keys are not present to avoid
regressions.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 68681abd-948f-4617-a53f-2e788886987a
📒 Files selected for processing (16)
src/pages/Heroes/api/HeroDetailDataSource.tssrc/pages/Items/api/ItemDetailDataSource.tssrc/pages/Locations/api/LocationDetailDataSource.tssrc/pages/Locations/model/LocationDetailViewModel.tssrc/pages/Locations/ui/LocationDetailPage/LocationDetailPage.tsxsrc/pages/Monsters/api/MonsterDetailDataSource.tssrc/pages/Npcs/api/NpcDetailDataSource.tssrc/pages/Npcs/model/NpcDetailViewModel.tssrc/pages/Npcs/ui/NpcDetailPage/NpcDetailPage.tsxsrc/pages/Parties/api/PartyDetailDataSource.tssrc/pages/Quests/api/QuestDetailDataSource.tssrc/pages/Regions/api/RegionDetailDataSource.tssrc/pages/Regions/model/RegionDetailViewModel.tssrc/pages/Regions/ui/RegionDetailPage/RegionDetailPage.tsxsrc/shared/lib/helpers/graphqlErrors.tssrc/shared/lib/index.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (15)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.agents/skills/frontend-self-review/SKILL.md)
**/*.{ts,tsx}: React views should render observable state and forward events only, without managing state directly
Repeated rows/cards should use List and Item ViewModel boundaries when formatting, links, badges, actions, permissions, files, or computed labels are involved
Methods should stay at one abstraction level
One non-trivial React component should live in one file
**/*.{ts,tsx}: React components render observable model state and forward events only. ViewModels own page state, actions, derived values, transitions, and descriptors.
DataSources own GraphQL SDK calls, request lifecycle, response extraction, pagination, cancellation, and transport-level error mapping.
Small bootstrap pages may callApiServicedirectly until the page spec is markedDone. Pages markedDonemust use a DataSource for non-trivial API access.
Keep methods at one abstraction level.
Files:
src/shared/lib/index.tssrc/pages/Locations/ui/LocationDetailPage/LocationDetailPage.tsxsrc/pages/Regions/ui/RegionDetailPage/RegionDetailPage.tsxsrc/pages/Parties/api/PartyDetailDataSource.tssrc/pages/Locations/model/LocationDetailViewModel.tssrc/pages/Npcs/ui/NpcDetailPage/NpcDetailPage.tsxsrc/pages/Npcs/model/NpcDetailViewModel.tssrc/shared/lib/helpers/graphqlErrors.tssrc/pages/Regions/model/RegionDetailViewModel.tssrc/pages/Locations/api/LocationDetailDataSource.tssrc/pages/Heroes/api/HeroDetailDataSource.tssrc/pages/Regions/api/RegionDetailDataSource.tssrc/pages/Items/api/ItemDetailDataSource.tssrc/pages/Npcs/api/NpcDetailDataSource.tssrc/pages/Monsters/api/MonsterDetailDataSource.tssrc/pages/Quests/api/QuestDetailDataSource.ts
src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (REVIEW.md)
Keep FSD dependencies one-way:
app -> pages -> widgets/features/entities -> shared
Files:
src/shared/lib/index.tssrc/pages/Locations/ui/LocationDetailPage/LocationDetailPage.tsxsrc/pages/Regions/ui/RegionDetailPage/RegionDetailPage.tsxsrc/pages/Parties/api/PartyDetailDataSource.tssrc/pages/Locations/model/LocationDetailViewModel.tssrc/pages/Npcs/ui/NpcDetailPage/NpcDetailPage.tsxsrc/pages/Npcs/model/NpcDetailViewModel.tssrc/shared/lib/helpers/graphqlErrors.tssrc/pages/Regions/model/RegionDetailViewModel.tssrc/pages/Locations/api/LocationDetailDataSource.tssrc/pages/Heroes/api/HeroDetailDataSource.tssrc/pages/Regions/api/RegionDetailDataSource.tssrc/pages/Items/api/ItemDetailDataSource.tssrc/pages/Npcs/api/NpcDetailDataSource.tssrc/pages/Monsters/api/MonsterDetailDataSource.tssrc/pages/Quests/api/QuestDetailDataSource.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (REVIEW.md)
src/**/*.{ts,tsx}: Prefer small methods with one abstraction level: public actions orchestrate; private helpers transform data, build variables, build item models, or map state, but not all at once
Shared or persisted state must be implemented with DI services or dedicated state ViewModels, not ad hoc module variables or direct browser storage calls in React components
Files:
src/shared/lib/index.tssrc/pages/Locations/ui/LocationDetailPage/LocationDetailPage.tsxsrc/pages/Regions/ui/RegionDetailPage/RegionDetailPage.tsxsrc/pages/Parties/api/PartyDetailDataSource.tssrc/pages/Locations/model/LocationDetailViewModel.tssrc/pages/Npcs/ui/NpcDetailPage/NpcDetailPage.tsxsrc/pages/Npcs/model/NpcDetailViewModel.tssrc/shared/lib/helpers/graphqlErrors.tssrc/pages/Regions/model/RegionDetailViewModel.tssrc/pages/Locations/api/LocationDetailDataSource.tssrc/pages/Heroes/api/HeroDetailDataSource.tssrc/pages/Regions/api/RegionDetailDataSource.tssrc/pages/Items/api/ItemDetailDataSource.tssrc/pages/Npcs/api/NpcDetailDataSource.tssrc/pages/Monsters/api/MonsterDetailDataSource.tssrc/pages/Quests/api/QuestDetailDataSource.ts
src/**/*.ts
⚙️ CodeRabbit configuration file
src/**/*.ts: Review against REVIEW.md and docs/architecture/frontend.md.
Block architecture drift: React components must not fetch, build query variables,
own business state, perform business decisions, or access persisted/browser state
directly. Page state, actions, derived values, and ExplainerDescriptor construction
belong in ViewModels. Non-trivial API access belongs in DataSources, except
small bootstrap pages may call ApiService directly until the page spec is marked
Done. Repeated row/card/detail surfaces should use Item ViewModels when rows have
formatting, links, badges, actions, permissions, files, or computed labels.
Prefer MobX computed getters for values read by views. Methods should stay at
one abstraction level and separate orchestration, variable building, response
mapping, and display formatting.
Files:
src/shared/lib/index.tssrc/pages/Parties/api/PartyDetailDataSource.tssrc/pages/Locations/model/LocationDetailViewModel.tssrc/pages/Npcs/model/NpcDetailViewModel.tssrc/shared/lib/helpers/graphqlErrors.tssrc/pages/Regions/model/RegionDetailViewModel.tssrc/pages/Locations/api/LocationDetailDataSource.tssrc/pages/Heroes/api/HeroDetailDataSource.tssrc/pages/Regions/api/RegionDetailDataSource.tssrc/pages/Items/api/ItemDetailDataSource.tssrc/pages/Npcs/api/NpcDetailDataSource.tssrc/pages/Monsters/api/MonsterDetailDataSource.tssrc/pages/Quests/api/QuestDetailDataSource.ts
src/**/{pages,widgets,features}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (REVIEW.md)
List/card/detail rendering must use explicit List ViewModel and Item ViewModel boundaries when rows need links, formatting, permissions, or actions
Files:
src/pages/Locations/ui/LocationDetailPage/LocationDetailPage.tsxsrc/pages/Regions/ui/RegionDetailPage/RegionDetailPage.tsxsrc/pages/Parties/api/PartyDetailDataSource.tssrc/pages/Locations/model/LocationDetailViewModel.tssrc/pages/Npcs/ui/NpcDetailPage/NpcDetailPage.tsxsrc/pages/Npcs/model/NpcDetailViewModel.tssrc/pages/Regions/model/RegionDetailViewModel.tssrc/pages/Locations/api/LocationDetailDataSource.tssrc/pages/Heroes/api/HeroDetailDataSource.tssrc/pages/Regions/api/RegionDetailDataSource.tssrc/pages/Items/api/ItemDetailDataSource.tssrc/pages/Npcs/api/NpcDetailDataSource.tssrc/pages/Monsters/api/MonsterDetailDataSource.tssrc/pages/Quests/api/QuestDetailDataSource.ts
src/**/pages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (REVIEW.md)
API access for a non-trivial page must use a DataSource and not bypass it by talking directly to low-level clients from a component or large page ViewModel method
Files:
src/pages/Locations/ui/LocationDetailPage/LocationDetailPage.tsxsrc/pages/Regions/ui/RegionDetailPage/RegionDetailPage.tsxsrc/pages/Parties/api/PartyDetailDataSource.tssrc/pages/Locations/model/LocationDetailViewModel.tssrc/pages/Npcs/ui/NpcDetailPage/NpcDetailPage.tsxsrc/pages/Npcs/model/NpcDetailViewModel.tssrc/pages/Regions/model/RegionDetailViewModel.tssrc/pages/Locations/api/LocationDetailDataSource.tssrc/pages/Heroes/api/HeroDetailDataSource.tssrc/pages/Regions/api/RegionDetailDataSource.tssrc/pages/Items/api/ItemDetailDataSource.tssrc/pages/Npcs/api/NpcDetailDataSource.tssrc/pages/Monsters/api/MonsterDetailDataSource.tssrc/pages/Quests/api/QuestDetailDataSource.ts
src/**/ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.agents/rules/frontend-mvvm-boundaries.mdc)
src/**/ui/**/*.{ts,tsx}: Use List ViewModel and Item ViewModel boundaries for non-trivial repeated rows/cards.
Prefer MobX computed getters for values read by views.
src/**/ui/**/*.{ts,tsx}: Frontend architecture contract: Keep React views thin, with state/actions/derived values owned by ViewModels
Frontend architecture contract: One non-trivial React component per file
Frontend architecture contract: Use same-named folders for non-test component files under src/**/ui/ with no component-folder index.ts barrels
Files:
src/pages/Locations/ui/LocationDetailPage/LocationDetailPage.tsxsrc/pages/Regions/ui/RegionDetailPage/RegionDetailPage.tsxsrc/pages/Npcs/ui/NpcDetailPage/NpcDetailPage.tsx
src/**/ui/*/*.{ts,tsx}
📄 CodeRabbit inference engine (.agents/rules/frontend-mvvm-boundaries.mdc)
Every non-test component file under
src/**/ui/must live in a same-named folder, for exampleui/RegionCard/RegionCard.tsx. Do not add component-folder barrels such asui/RegionCard/index.ts.
Files:
src/pages/Locations/ui/LocationDetailPage/LocationDetailPage.tsxsrc/pages/Regions/ui/RegionDetailPage/RegionDetailPage.tsxsrc/pages/Npcs/ui/NpcDetailPage/NpcDetailPage.tsx
src/**/*.tsx
⚙️ CodeRabbit configuration file
src/**/*.tsx: Review React components as thin observer views. They should render observable
model state and forward events to ViewModel actions only. Flag JSX that contains
request orchestration, filter/sort/query payload construction, link-building
decisions, business rules, direct storage/browser access, or raw GraphQL response
transformations. Enforce one non-trivial React component per file. Enforce
same-named folders for non-test component files under src/**/ui/ and reject
component-folder index.ts barrels.
Files:
src/pages/Locations/ui/LocationDetailPage/LocationDetailPage.tsxsrc/pages/Regions/ui/RegionDetailPage/RegionDetailPage.tsxsrc/pages/Npcs/ui/NpcDetailPage/NpcDetailPage.tsx
**/*DataSource.{ts,tsx}
📄 CodeRabbit inference engine (.agents/skills/frontend-self-review/SKILL.md)
DataSources should own SDK calls, request lifecycle, response extraction, pagination, and transport-level error mapping
Files:
src/pages/Parties/api/PartyDetailDataSource.tssrc/pages/Locations/api/LocationDetailDataSource.tssrc/pages/Heroes/api/HeroDetailDataSource.tssrc/pages/Regions/api/RegionDetailDataSource.tssrc/pages/Items/api/ItemDetailDataSource.tssrc/pages/Npcs/api/NpcDetailDataSource.tssrc/pages/Monsters/api/MonsterDetailDataSource.tssrc/pages/Quests/api/QuestDetailDataSource.ts
src/**/*DataSource*.{ts,tsx}
📄 CodeRabbit inference engine (REVIEW.md)
DataSources own GraphQL SDK calls, request cancellation/reset, response extraction, pagination shape, and transport-level error mapping
Files:
src/pages/Parties/api/PartyDetailDataSource.tssrc/pages/Locations/api/LocationDetailDataSource.tssrc/pages/Heroes/api/HeroDetailDataSource.tssrc/pages/Regions/api/RegionDetailDataSource.tssrc/pages/Items/api/ItemDetailDataSource.tssrc/pages/Npcs/api/NpcDetailDataSource.tssrc/pages/Monsters/api/MonsterDetailDataSource.tssrc/pages/Quests/api/QuestDetailDataSource.ts
**/*ViewModel.{ts,tsx}
📄 CodeRabbit inference engine (.agents/skills/frontend-self-review/SKILL.md)
**/*ViewModel.{ts,tsx}: ViewModels should own state, actions, derived values, transitions, and descriptors
Prefer computed getters for derived read models
Files:
src/pages/Locations/model/LocationDetailViewModel.tssrc/pages/Npcs/model/NpcDetailViewModel.tssrc/pages/Regions/model/RegionDetailViewModel.ts
src/**/pages/**/*ViewModel*.{ts,tsx}
📄 CodeRabbit inference engine (REVIEW.md)
Page ViewModels own request lifecycle, filters, cursor, locale, state transitions, actions, derived values, and ExplainerDescriptor construction
Files:
src/pages/Locations/model/LocationDetailViewModel.tssrc/pages/Npcs/model/NpcDetailViewModel.tssrc/pages/Regions/model/RegionDetailViewModel.ts
src/**/{model,viewModel}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (REVIEW.md)
Favor computed getters for read models and avoid keeping duplicate mutable state when it can be derived from request data, filters, route params, or services
Files:
src/pages/Locations/model/LocationDetailViewModel.tssrc/pages/Npcs/model/NpcDetailViewModel.tssrc/pages/Regions/model/RegionDetailViewModel.ts
src/**/*ViewModel*.{ts,tsx}
📄 CodeRabbit inference engine (REVIEW.md)
src/**/*ViewModel*.{ts,tsx}: Derived display state must be exposed as MobX computed getters where the value is read by the view, not recomputed imperatively
A ViewModel must not mix unrelated abstraction levels in the same method, such as request orchestration, GraphQL response parsing, URL building, and JSX display decisions in one block
Files:
src/pages/Locations/model/LocationDetailViewModel.tssrc/pages/Npcs/model/NpcDetailViewModel.tssrc/pages/Regions/model/RegionDetailViewModel.ts
🧠 Learnings (1)
📚 Learning: 2026-05-20T08:50:13.209Z
Learnt from: anton62k
Repo: revisium/demo-rpg-frontend PR: 31
File: src/pages/Npcs/model/NpcDetailViewModel.ts:167-172
Timestamp: 2026-05-20T08:50:13.209Z
Learning: When building navigation for NPC pages (catalog or detail) under `src/pages/Npcs/`, call `getSectionNavigationItems('heroes')`—do not call it with `'npcs'`. This is because `src/shared/config/navigation.ts`’s `SectionNavKey` only defines top-level groups (`heroes`, `items`, `quests`, `world`), and `npcs` is a sub-item inside the `heroes` group.
Applied to files:
src/pages/Npcs/ui/NpcDetailPage/NpcDetailPage.tsxsrc/pages/Npcs/model/NpcDetailViewModel.tssrc/pages/Npcs/api/NpcDetailDataSource.ts
🔇 Additional comments (15)
src/pages/Heroes/api/HeroDetailDataSource.ts (1)
2-2: LGTM!Also applies to: 6-6, 9-9, 30-30
src/pages/Items/api/ItemDetailDataSource.ts (1)
2-2: LGTM!Also applies to: 5-5, 9-9, 24-30
src/pages/Monsters/api/MonsterDetailDataSource.ts (1)
2-2: LGTM!Also applies to: 5-5, 10-10, 25-31
src/pages/Parties/api/PartyDetailDataSource.ts (1)
2-2: LGTM!Also applies to: 5-5, 9-9, 24-30
src/pages/Quests/api/QuestDetailDataSource.ts (1)
2-2: LGTM!Also applies to: 5-5, 10-10, 25-31
src/pages/Npcs/api/NpcDetailDataSource.ts (1)
2-2: LGTM!Also applies to: 5-5, 8-8, 23-29
src/pages/Npcs/model/NpcDetailViewModel.ts (1)
163-165: LGTM!Also applies to: 168-168
src/pages/Npcs/ui/NpcDetailPage/NpcDetailPage.tsx (1)
39-45: LGTM!src/pages/Regions/api/RegionDetailDataSource.ts (1)
2-2: LGTM!Also applies to: 5-5, 8-8, 23-29
src/pages/Regions/model/RegionDetailViewModel.ts (1)
178-180: LGTM!Also applies to: 183-183
src/pages/Regions/ui/RegionDetailPage/RegionDetailPage.tsx (1)
39-45: LGTM!src/shared/lib/helpers/graphqlErrors.ts (1)
1-27: LGTM!src/shared/lib/index.ts (1)
47-47: LGTM!src/pages/Locations/api/LocationDetailDataSource.ts (1)
2-2: LGTM!Also applies to: 5-5, 8-8, 23-29
src/pages/Locations/model/LocationDetailViewModel.ts (1)
215-217: LGTM!
|
|
||
| public get showDetail(): boolean { | ||
| return Boolean(this.item) && !this.showError; | ||
| return Boolean(this.item) && !this.showError && !this.showNotFound; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
The !this.showNotFound check is redundant.
The condition Boolean(this.item) already guarantees showNotFound will be false, since showNotFound requires !this.item. These are mutually exclusive states.
♻️ Simplify the guard
public get showDetail(): boolean {
- return Boolean(this.item) && !this.showError && !this.showNotFound;
+ return Boolean(this.item) && !this.showError;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/Locations/model/LocationDetailViewModel.ts` at line 220, The return
expression in LocationDetailViewModel uses a redundant check: remove the
unnecessary && !this.showNotFound from the guard (keep Boolean(this.item) &&
!this.showError) because showNotFound is already derived from !this.item; update
the return in the getter/method that currently returns "Boolean(this.item) &&
!this.showError && !this.showNotFound" to "Boolean(this.item) &&
!this.showError" so behavior remains the same but the condition is simplified.
| {renderWhen( | ||
| vm.showNotFound, | ||
| <StatePanel | ||
| description={vm.copy.detail.errorDescription} | ||
| title={vm.copy.detail.errorTitle} | ||
| />, | ||
| )} |
There was a problem hiding this comment.
Not-found state uses generic error copy.
The not-found panel renders vm.copy.detail.errorTitle and vm.copy.detail.errorDescription, which are designed for generic errors. A missing resource is a normal state, not an error condition, and should explain that the location wasn't found rather than suggesting something went wrong.
Consider adding dedicated not-found copy in the ViewModel's copy structure, such as vm.copy.detail.notFoundTitle and vm.copy.detail.notFoundDescription, to provide clearer user feedback that differentiates this state from actual errors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/Locations/ui/LocationDetailPage/LocationDetailPage.tsx` around
lines 40 - 46, The not-found branch is using generic error copy
(vm.copy.detail.errorTitle / errorDescription) which implies a failure; update
the not-found rendering to use dedicated not-found copy keys (e.g.,
vm.copy.detail.notFoundTitle and vm.copy.detail.notFoundDescription) in the
renderWhen(vm.showNotFound, <StatePanel ... />) call and update the ViewModel to
supply those keys; also add a safe fallback to the existing
errorTitle/errorDescription if the new notFound keys are not present to avoid
regressions.
8a4e1b8 to
5a5c47c
Compare
5a5c47c to
a2a1bb6
Compare
|
|
Ready for review. Validation passed locally:
GitHub CI, SonarCloud, and CodeRabbit are green. The PR fixes the generated GraphQL NOT_FOUND normalization for the reproduced Regions detail route and existing detail pages with not-found state while preserving non-404 errors. |
1 participant
Fixes #38
Summary
NOT_FOUNDnormalization helpersitem: nullfor the reproduced Regions detail route and detail routes that already expose a not-found stateValidation
npm run ts:checknpm run lint:cinpm run ui:lint$env:CHOKIDAR_USEPOLLING='1'; npx steiger srcnpm run buildgit diff --check