Feat/world map - #401
Merged
Merged
Conversation
…-existing PLR0917
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
for more information, see https://pre-commit.ci
Contributor
|
Warning Review limit reached
Next review available in: 12 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughVersion 2.24.0 adds persisted wasteland locations, procedural discovery events, map APIs, resilient dweller-place registration, deterministic vault markers, and an authenticated frontend world-map experience with polling and marker details. ChangesWorld Map and Discovery
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant MapView
participant useMapStore
participant MapAPI
participant MapService
participant Database
User->>MapView: Open vault map
MapView->>useMapStore: fetchMap(vaultId, token)
useMapStore->>MapAPI: GET vault map
MapAPI->>MapService: get_vault_map
MapService->>Database: Load locations and dweller references
Database-->>MapService: Map data
MapService-->>MapAPI: VaultMapResponse
MapAPI-->>useMapStore: VaultMapResponse
useMapStore-->>MapView: Render locations and vault markers
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (6)
backend/app/tests/test_api/test_system.py (1)
31-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the runtime version instead of a second literal.
The assertion hard-codes
"2.24.0", whiletest_get_changelog_latest_matches_app_versionalready callsget_app_version(). A future version bump will require editing this assertion even whenbackend/pyproject.tomlandCHANGELOG.mdare consistent. Comparedata["version"]withget_app_version()here as well.Proposed change
+ from app.utils.version import get_app_version + + expected_version = get_app_version() - assert data["version"] == "2.24.0", ( + assert data["version"] == expected_version, (🤖 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 `@backend/app/tests/test_api/test_system.py` around lines 31 - 37, Update test_get_changelog_latest_matches_app_version to compare data["version"] with the existing get_app_version() result instead of the hard-coded "2.24.0" literal, while preserving the current mismatch error context and other assertions.backend/app/tests/test_utils/test_places.py (1)
17-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd return types to the test methods.
Each new
test_*method omits-> None. Add the return annotation to every test method in this file.As per coding guidelines,
backend/**/*.pyrequires: “Add type hints to new functions and prefer UUID4/Pydantic types where applicable.”🤖 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 `@backend/app/tests/test_utils/test_places.py` around lines 17 - 139, Add the return annotation -> None to every test_* method shown in TestSchematicCoords, TestCollisionNudge, and TestSeededVaultSpecs, as well as the preceding normalization tests. Do not change test behavior or method parameters.Source: Coding guidelines
backend/app/services/dweller_ai.py (1)
52-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the suppressed exception instead of discarding it.
with suppress(Exception)hides every failure in this helper without a log record.map_service.register_bio_placeslogs its own internal failures, but any error raised outside that scope (for example an attribute error ondweller_obj) disappears. Replace the suppression with anexceptclause that callslogger.exception.As per coding guidelines: "log from services with
logging.getLogger(__name__)and uselogger.exceptionfor unexpected errors."♻️ Proposed change to log the failure
"""Register bio-extracted places on the world map — best-effort, never raises.""" - with suppress(Exception): - await map_service.register_bio_places( - db_session, - dweller_obj, - origin_place=origin_place, - visited_places=visited_places, - explicit_origin=explicit_origin, - ) + try: + await map_service.register_bio_places( + db_session, + dweller_obj, + origin_place=origin_place, + visited_places=visited_places, + explicit_origin=explicit_origin, + ) + except Exception: + logger.exception("Map place registration failed for dweller %s", dweller_obj.id)Remove the now unused
from contextlib import suppressimport at line 3.🤖 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 `@backend/app/services/dweller_ai.py` around lines 52 - 59, Replace the suppress(Exception) wrapper around map_service.register_bio_places with explicit exception handling that calls logger.exception for unexpected failures, preserving the helper’s non-raising behavior. Ensure the module defines logger via logging.getLogger(__name__), and remove the now-unused contextlib.suppress import.Source: Coding guidelines
backend/app/tests/test_services/test_discovery_events.py (1)
44-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the loops produce events, so the tests cannot pass vacuously.
Both loops call
continuewhengenerate_eventreturnsNone. If the generator gates on the time since the last recorded event, thenadd_eventinside the loop can suppress every later draw. In that casetest_no_discovery_when_chance_is_0verifies far fewer than 200 draws, andtest_discovery_event_generated_when_chance_is_1can assert nothing at all. Count the generated events and assert the count is greater than zero.💚 Proposed change for the chance-1.0 test
with patch.object(game_config.exploration, "event_discovery_chance", 1.0): + generated = 0 for _ in range(20): event = event_generator.generate_event(exploration) if event is None: continue + generated += 1 assert isinstance(event, DiscoveryEventSchema), f"Expected discovery, got {type(event).__name__}"exploration.add_event( event_type=event.type, description=event.description, location_name=event.location_name, ) + assert generated > 0, "No events were generated; the assertions never ran"Apply the same counter to
test_no_discovery_when_chance_is_0.Also applies to: 121-132
🤖 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 `@backend/app/tests/test_services/test_discovery_events.py` around lines 44 - 60, Update both discovery-event tests around event_generator.generate_event in the chance-1.0 and chance-0.0 loops to count non-None generated events, incrementing the counter before continuing or processing the event, and assert the count is greater than zero after each loop so neither test can pass vacuously.backend/app/services/breeding_service.py (1)
344-344: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse one vault lookup and extract the post-delivery side effects.
deliver_babynow loads the same vault twice: at line 438 throughvault_crud_for_mapand at line 461 throughvault_crud. Both are the same CRUD object imported under two aliases with the same argumentmother.vault_id. Load the vault once and pass it to both blocks.The
# noqa: PLR0915at line 344 suppresses the statement-count rule instead of reducing the function. Extracting the two best-effort post-delivery blocks into private helpers removes the need for the suppression.♻️ Proposed helper extraction
`@staticmethod` async def _link_newborn_to_home(db_session: AsyncSession, child: Dweller, vault) -> None: """Link the newborn to the vault home marker — best-effort.""" from app.services.map_service import map_service try: await map_service.link_home_origin(db_session, child, vault) except Exception: logger.exception( "Failed to link home origin for newborn: child=%s vault=%s", child.id, vault.id )- # Link newborn to home vault on world map (best-effort, non-critical) - try: - from app.crud.vault import vault as vault_crud_for_map - from app.services.map_service import map_service - - vault_for_map = await vault_crud_for_map.get(db_session, mother.vault_id) - if vault_for_map: - await map_service.link_home_origin(db_session, child, vault_for_map) - except Exception: - logger.exception( - "Failed to link home origin for newborn: child=%s vault=%s", - child.id, - mother.vault_id, - ) + from app.crud.vault import vault as vault_crud + + vault = await vault_crud.get(db_session, mother.vault_id) + if vault: + await BreedingService._link_newborn_to_home(db_session, child, vault)Then reuse the same
vaultvalue in the statistics block instead of loading it again.Also applies to: 433-447
🤖 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 `@backend/app/services/breeding_service.py` at line 344, Update deliver_baby to remove the PLR0915 suppression, load mother.vault_id once into a shared vault value, and reuse it in both post-delivery blocks instead of calling vault_crud_for_map and vault_crud separately. Extract the best-effort home-linking and statistics side effects into private helpers, including _link_newborn_to_home with exception logging, and invoke those helpers from deliver_baby while preserving existing behavior.backend/app/services/map_service.py (1)
99-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWiden or correct the
dwellerparameter type.
register_bio_placesdeclaresdweller: Dweller. The production caller inbackend/app/services/dweller_ai.py(lines 42-59) passesdweller_obj: DwellerReadFull. The method only readsdweller.idanddweller.vault_id, so runtime works, but the annotation is incorrect for the main caller. Declare aProtocolwithidandvault_id, or acceptDweller | DwellerReadFull, or accept the two identifiers directly.As per coding guidelines: "Add type hints to new functions and prefer UUID4/Pydantic types where applicable."
♻️ Proposed change to accept identifiers explicitly
- async def register_bio_places( - self, - db_session: AsyncSession, - dweller: Dweller, - origin_place: str, - visited_places: list[str], - explicit_origin: str | None = None, - ) -> None: + async def register_bio_places( + self, + db_session: AsyncSession, + dweller: Dweller | DwellerReadFull, + origin_place: str, + visited_places: list[str], + explicit_origin: str | None = None, + ) -> None:🤖 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 `@backend/app/services/map_service.py` around lines 99 - 106, Correct the dweller parameter typing in register_bio_places to match its production caller and actual usage of only id and vault_id. Prefer a small Protocol exposing those attributes, or explicitly support both Dweller and DwellerReadFull, while preserving the existing method behavior.Source: Coding guidelines
🤖 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
`@backend/app/alembic/versions/2026_08_06_2353-edb924d8dbeb_add_wasteland_locations.py`:
- Around line 47-48: Add named database CHECK constraints enforcing coord_x and
coord_y values from 0 through 100 in
backend/app/alembic/versions/2026_08_06_2353-edb924d8dbeb_add_wasteland_locations.py:47-48.
Add matching sa.CheckConstraint entries to WastelandLocation.__table_args__
alongside the unique constraint in
backend/app/models/wasteland_location.py:34-35, and add commit-level tests
rejecting negative and greater-than-100 coordinates in
backend/app/tests/test_models/test_wasteland_location.py:62-88.
In `@backend/app/api/v1/endpoints/map.py`:
- Around line 37-56: Move the CRUD lookup, vault ownership validation, dweller
reference retrieval, and WastelandLocationWithDwellers construction out of the
endpoint’s get_location_detail function into a map_service.get_location_detail
method. Keep the endpoint limited to parsing dependencies/parameters and
delegating to the service, preserving the existing not-found behavior and
response fields.
In `@backend/app/crud/wasteland_location.py`:
- Around line 72-103: Update the WastelandLocation model/schema to add a unique
constraint covering vault_id, coord_x, and coord_y, and ensure the corresponding
migration is included. In the creation flow around collision_nudge and the
IntegrityError handler, distinguish coordinate conflicts from normalized-name
conflicts, retry coordinate selection and insertion after rolling back, and
retain the existing get_by_normalized lookup for name conflicts.
In `@backend/app/schemas/dweller_ai.py`:
- Around line 20-24: Update both visited_places fields in the relevant schema to
apply a max_length of 64 to each string item using an item-level constrained
type such as Annotated[str, Field(max_length=64)]. Preserve the existing
list-level max_length of 5 and field defaults while ensuring names exceeding 64
characters are rejected before MapService.register_bio_places.
In `@backend/app/services/map_service.py`:
- Around line 76-85: The map helper transaction handling in
get_or_create/link_dweller must not commit or roll back the caller’s
AsyncSession, because register_discovery may have pending Exploration work. Move
map create/link operations to an independent session or an isolated nested
transaction so only map changes are committed or rolled back while the caller
transaction remains intact.
In `@backend/app/tests/test_services/test_newborn_origin.py`:
- Around line 78-86: Add UUID4 annotations to mother_id and father_id in
_create_due_pregnancy, and annotate its return value with the pregnancy model
type returned by BreedingService.create_pregnancy. Add -> None to both async
test functions in the affected section, preserving their existing behavior.
In `@frontend/src/modules/map/components/MapMarker.vue`:
- Around line 44-59: Make the map actions keyboard accessible: in
frontend/src/modules/map/components/MapMarker.vue lines 44-59, make the marker
<g> focusable and trigger its existing click emission on Enter and Space; in
frontend/src/modules/map/components/WorldMap.vue lines 45-46, remove the
whole-SVG role="img" aria-label treatment so interactive MapMarker children
remain accessible; in frontend/src/modules/map/components/MarkerDetailModal.vue
lines 92-100, replace each dweller navigation action with a button supporting
native Enter/Space activation.
In `@frontend/src/modules/map/stores/map.ts`:
- Around line 16-69: Update the polling callback created by useIntervalFn and
the startPolling/stopPolling lifecycle to prevent stale responses from
committing map state: capture the vault ID and token when each request starts,
assign it a monotonically increasing generation, and only update locations and
vaultMarkers when the generation and captured context still match the active
polling context. Invalidate the generation when polling is switched or stopped,
and add a deferred-promise test covering a request resolving after a context
change or stop.
In `@frontend/src/modules/map/views/MapView.vue`:
- Around line 77-82: Update the MapView template’s conditional rendering around
hasNoData to check mapStore.error before showing the empty state, displaying the
request failure message instead of “uncharted.” Add a retry action that invokes
the existing map-fetch flow, while preserving the current empty-state behavior
for successful responses with no map data.
- Around line 42-48: Update the MapView.vue lifecycle around onMounted to watch
vaultId changes, stopping the existing mapStore polling context before fetching
and starting polling for the new vault. Guard fetch results and mapStore commits
so delayed responses from a previous vault cannot overwrite the currently
selected vault’s map.
In `@ROADMAP.md`:
- Line 14: Update the v2.25.0 roadmap entry to describe the actual PostgreSQL
enum synchronization path: do not imply that Alembic’s compare_type=True
generates enum value migrations; document the explicit migration strategy or
enum autogenerate extension, including ALTER TYPE handling for added/renamed
labels and the required replacement approach for removed labels. Add regression
coverage for both added and removed enum labels.
---
Nitpick comments:
In `@backend/app/services/breeding_service.py`:
- Line 344: Update deliver_baby to remove the PLR0915 suppression, load
mother.vault_id once into a shared vault value, and reuse it in both
post-delivery blocks instead of calling vault_crud_for_map and vault_crud
separately. Extract the best-effort home-linking and statistics side effects
into private helpers, including _link_newborn_to_home with exception logging,
and invoke those helpers from deliver_baby while preserving existing behavior.
In `@backend/app/services/dweller_ai.py`:
- Around line 52-59: Replace the suppress(Exception) wrapper around
map_service.register_bio_places with explicit exception handling that calls
logger.exception for unexpected failures, preserving the helper’s non-raising
behavior. Ensure the module defines logger via logging.getLogger(__name__), and
remove the now-unused contextlib.suppress import.
In `@backend/app/services/map_service.py`:
- Around line 99-106: Correct the dweller parameter typing in
register_bio_places to match its production caller and actual usage of only id
and vault_id. Prefer a small Protocol exposing those attributes, or explicitly
support both Dweller and DwellerReadFull, while preserving the existing method
behavior.
In `@backend/app/tests/test_api/test_system.py`:
- Around line 31-37: Update test_get_changelog_latest_matches_app_version to
compare data["version"] with the existing get_app_version() result instead of
the hard-coded "2.24.0" literal, while preserving the current mismatch error
context and other assertions.
In `@backend/app/tests/test_services/test_discovery_events.py`:
- Around line 44-60: Update both discovery-event tests around
event_generator.generate_event in the chance-1.0 and chance-0.0 loops to count
non-None generated events, incrementing the counter before continuing or
processing the event, and assert the count is greater than zero after each loop
so neither test can pass vacuously.
In `@backend/app/tests/test_utils/test_places.py`:
- Around line 17-139: Add the return annotation -> None to every test_* method
shown in TestSchematicCoords, TestCollisionNudge, and TestSeededVaultSpecs, as
well as the preceding normalization tests. Do not change test behavior or method
parameters.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 721bd47d-a588-405b-8daa-a16d60da2492
⛔ Files ignored due to path filters (2)
backend/uv.lockis excluded by!**/*.lockfrontend/src/core/types/api.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (53)
CHANGELOG.mdROADMAP.mdbackend/app/agents/dweller_agents.pybackend/app/alembic/versions/2026_08_06_2353-edb924d8dbeb_add_wasteland_locations.pybackend/app/api/v1/api.pybackend/app/api/v1/endpoints/map.pybackend/app/core/game_config.pybackend/app/crud/__init__.pybackend/app/crud/wasteland_location.pybackend/app/data/exploration/discovery_names.jsonbackend/app/models/__init__.pybackend/app/models/exploration.pybackend/app/models/wasteland_location.pybackend/app/schemas/dweller_ai.pybackend/app/schemas/exploration_event.pybackend/app/schemas/wasteland_location.pybackend/app/services/breeding_service.pybackend/app/services/dweller_ai.pybackend/app/services/exploration/coordinator.pybackend/app/services/exploration/data_loader.pybackend/app/services/exploration/event_generator.pybackend/app/services/map_service.pybackend/app/tests/integration/test_quota_enforcement.pybackend/app/tests/test_api/test_map.pybackend/app/tests/test_api/test_system.pybackend/app/tests/test_crud/test_wasteland_location.pybackend/app/tests/test_models/test_wasteland_location.pybackend/app/tests/test_services/test_discovery_events.pybackend/app/tests/test_services/test_dweller_ai_map.pybackend/app/tests/test_services/test_exploration_service.pybackend/app/tests/test_services/test_map_service.pybackend/app/tests/test_services/test_newborn_origin.pybackend/app/tests/test_utils/test_places.pybackend/app/utils/places.pybackend/pyproject.tomlfrontend/package.jsonfrontend/src/core/components/common/SidePanel.vuefrontend/src/modules/exploration/models/exploration.tsfrontend/src/modules/map/README.mdfrontend/src/modules/map/components/MapMarker.vuefrontend/src/modules/map/components/MarkerDetailModal.vuefrontend/src/modules/map/components/WorldMap.vuefrontend/src/modules/map/index.tsfrontend/src/modules/map/models/map.tsfrontend/src/modules/map/routes/index.tsfrontend/src/modules/map/services/mapService.tsfrontend/src/modules/map/stores/map.tsfrontend/src/modules/map/views/MapView.vuefrontend/src/router/index.tsfrontend/tests/unit/components/map/MarkerDetailModal.test.tsfrontend/tests/unit/components/map/WorldMap.test.tsfrontend/tests/unit/modules/map/routes.test.tsfrontend/tests/unit/stores/map.test.ts
Comment on lines
+72
to
+103
| # Gather occupied coordinates for this vault | ||
| occupied_result = await db_session.execute( | ||
| select(WastelandLocation.coord_x, WastelandLocation.coord_y).where(WastelandLocation.vault_id == vault_id) | ||
| ) | ||
| occupied: set[tuple[float, float]] = {(rx, ry) for rx, ry in occupied_result.all()} | ||
|
|
||
| coord_x, coord_y = collision_nudge((base_x, base_y), occupied) | ||
|
|
||
| obj = WastelandLocation( | ||
| name=name[:64], | ||
| normalized_name=normalized, | ||
| type=type, | ||
| coord_x=coord_x, | ||
| coord_y=coord_y, | ||
| description=description, | ||
| vault_id=vault_id, | ||
| exploration_id=exploration_id, | ||
| ) | ||
| db_session.add(obj) | ||
| try: | ||
| await db_session.commit() | ||
| await db_session.refresh(obj) | ||
| return obj | ||
| except IntegrityError: | ||
| # Race: another request already inserted this name | ||
| await db_session.rollback() | ||
| # Re-fetch the existing row | ||
| existing = await self.get_by_normalized(db_session, vault_id, normalized) | ||
| if existing is not None: | ||
| return existing | ||
| # Should not happen — re-raise if we still can't find it | ||
| raise |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make coordinate allocation atomic.
Lines 72-78 select a coordinate from an unreserved snapshot. Concurrent requests with different names that hash to the same coordinate can select the same point. Without a database uniqueness constraint, the map stores overlapping markers. With a uniqueness constraint, Lines 95-103 reselect only by normalized name and re-raise the coordinate conflict for one request.
Add a unique vault-coordinate constraint. Retry coordinate selection after a coordinate conflict. Keep the existing normalized-name lookup for name conflicts.
🤖 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 `@backend/app/crud/wasteland_location.py` around lines 72 - 103, Update the
WastelandLocation model/schema to add a unique constraint covering vault_id,
coord_x, and coord_y, and ensure the corresponding migration is included. In the
creation flow around collision_nudge and the IntegrityError handler, distinguish
coordinate conflicts from normalized-name conflicts, retry coordinate selection
and insertion after rolling back, and retain the existing get_by_normalized
lookup for name conflicts.
Comment on lines
+76
to
+85
| try: | ||
| await db_session.commit() | ||
| await db_session.refresh(obj) | ||
| return obj | ||
| except IntegrityError: | ||
| await db_session.rollback() | ||
| existing = await wl_crud.get_by_normalized(db_session, vault.id, normalized) | ||
| if existing is not None: | ||
| return existing | ||
| raise |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect commit/rollback usage in the wasteland_location CRUD and its callers.
set -euo pipefail
fd -t f 'wasteland_location.py' backend/app/crud --exec cat -n {}
echo "--- map_service call sites ---"
rg -n -C 4 'map_service\.(ensure_home_marker|link_home_origin|register_bio_places|register_discovery|get_vault_map)' backend/app --glob '!**/tests/**'Repository: ElderEvil/falloutProject
Length of output: 11529
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- map_service outline/size ---"
wc -l backend/app/services/map_service.py
ast-grep outline backend/app/services/map_service.py 2>/dev/null || true
echo "--- relevant map_service sections ---"
sed -n '1,180p' backend/app/services/map_service.py | cat -n
echo "--- breeding_service around ensure_dweller_location/link_home_origin call ---"
sed -n '380,470p' backend/app/services/breeding_service.py | cat -n
echo "--- exploration coordinator around discovery call ---"
sed -n '1,150p' backend/app/services/exploration/coordinator.py | cat -n
echo "--- dweller_ai map registration ---"
sed -n '1,100p' backend/app/services/dweller_ai.py | cat -n
echo "--- session dependencies and current autocommit/pending status ---"
rg -n -C 3 'create_async_session|async_session|AsyncSession|get_async_session|commit\(|rollback\(\)|begin|begin_nested|autocommit|expire_no_pending|is_active|pending' backend/app --glob '*.py' --glob '!**/tests/**'Repository: ElderEvil/falloutProject
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- locate db_session.py ---"
fd -t f 'session.py' backend/app/db
echo "--- get_async_session implementation ---"
while IFS= read -r file; do
echo "### $file"
sed -n '1,220p' "$file" | cat -n
done < <(fd -t f 'session.py' backend/app/db)
echo "--- breeding_service critical section ---"
sed -n '340,455p' backend/app/services/breeding_service.py | cat -n
echo "--- exploration coordinator critical context ---"
sed -n '1,140p' backend/app/services/exploration/coordinator.py | cat -n
echo "--- dweller_ai critical context ---"
sed -n '1,80p' backend/app/services/dweller_ai.py | cat -n
echo "--- focused map_service methods ---"
rg -n -C 8 'ensure_home_marker|link_home_origin|register_bio_places|register_discovery|get_vault_map' backend/app/services/map_service.py backend/app/services --glob '!**/tests/**'
echo "--- focused rollback/commit/transaction helpers ---"
rg -n -C 3 'async def .*rollback|rollback\(\)|begin\(|begin_nested\(|expire_no_pending|pending|is_active|autocommit|AsyncEngine|AsyncSession' backend/app/db backend/app --glob '*.py' --glob '!**/tests/**' --max-count 200Repository: ElderEvil/falloutProject
Length of output: 311
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- get_async_session implementation ---"
sed -n '1,220p' backend/app/db/session.py | cat -n
echo "--- breeding_service critical section ---"
sed -n '340,455p' backend/app/services/breeding_service.py | cat -n
echo "--- exploration coordinator critical context ---"
sed -n '1,140p' backend/app/services/exploration/coordinator.py | cat -n
echo "--- dweller_ai critical context ---"
sed -n '1,80p' backend/app/services/dweller_ai.py | cat -n
echo "--- focused map_service methods ---"
rg -n -C 8 'ensure_home_marker|link_home_origin|register_bio_places|register_discovery|get_vault_map' backend/app/services/map_service.py backend/app/services --glob '!**/tests/**'
echo "--- focused transaction helpers ---"
rg -n -C 3 'rollback\(\)|begin\(|begin_nested\(|expire_no_pending|pending|is_active|autocommit|AsyncEngine|AsyncSession' backend/app/db backend/app --glob '*.py' --glob '!**/tests/**' --max-count 200Repository: ElderEvil/falloutProject
Length of output: 50381
🏁 Script executed:
#!/bin/bash
tmplog=$(mktemp)
tmplog2=$(mktemp)
set -euo pipefail
echo "--- get_async_session implementation ---"
sed -n '1,220p' backend/app/db/session.py | cat -n > "$tmplog"
cat "$tmplog"
echo "--- targeted breeding_service context ---"
sed -n '390,450p' backend/app/services/breeding_service.py | cat -n > "$tmplog"
cat "$tmplog"
echo "--- targeted exploration coordinator context ---"
sed -n '55,130p' backend/app/services/exploration/coordinator.py | cat -n > "$tmplog"
cat "$tmplog"
echo "--- targeted dweller_ai context ---"
sed -n '40,65p' backend/app/services/dweller_ai.py | cat -n > "$tmplog"
cat "$tmplog"
echo "--- focused map_service methods ---"
rg -n -C 10 'ensure_home_marker|link_home_origin|register_bio_places|register_discovery|get_vault_map' backend/app/services/map_service.py > "$tmplog2"
cat "$tmplog2"
echo "--- focused transaction helpers ---"
rg -n -C 3 'rollback\(\)|begin\(|begin_nested\(|expire_no_pending|pending|is_active|autocommit|AsyncEngine|AsyncSession' backend/app/db backend/app/db/*.py backend/app -g '*.py' -g '!**/tests/**' | head -n 240Repository: ElderEvil/falloutProject
Length of output: 28940
Do not issue or discard transactions on the caller session inside best-effort map helpers.
get_async_session() yields a manual AsyncSession, and get_or_create() / link_dweller() commit. register_discovery() is called after _handle_loot_event adds an Exploration, and that added exploration is not yet committed. An IntegrityError in the map helper then rolls back the caller session and loses the pending exploration event. Move map create/link operations out of the caller transaction, or use a nested scope that only rolls back the map work.
🤖 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 `@backend/app/services/map_service.py` around lines 76 - 85, The map helper
transaction handling in get_or_create/link_dweller must not commit or roll back
the caller’s AsyncSession, because register_discovery may have pending
Exploration work. Move map create/link operations to an independent session or
an isolated nested transaction so only map changes are committed or rolled back
while the caller transaction remains intact.
…ith conflict retry
… tidy breeding/dweller services
….25.0 roadmap entry
…rd, vault-aware MapView
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
for more information, see https://pre-commit.ci
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@backend/app/tests/test_utils/test_dweller_ai_schemas.py`:
- Around line 9-35: Add a -> None return annotation to each new test function:
test_dweller_backstory_visited_places_65_chars_rejected,
test_dweller_backstory_visited_places_64_chars_valid, and
test_extended_bio_visited_places_65_chars_rejected. Do not alter their test
logic.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b3b83c20-ba0d-4e55-9871-5410f65753c2
📒 Files selected for processing (24)
ROADMAP.mdbackend/app/alembic/versions/2026_08_07_1023-4408ea93bd5b_add_wasteland_location_coord_constraints.pybackend/app/api/v1/endpoints/map.pybackend/app/crud/wasteland_location.pybackend/app/models/wasteland_location.pybackend/app/schemas/dweller_ai.pybackend/app/services/breeding_service.pybackend/app/services/dweller_ai.pybackend/app/services/map_service.pybackend/app/tests/test_api/test_system.pybackend/app/tests/test_crud/test_wasteland_location.pybackend/app/tests/test_models/test_wasteland_location.pybackend/app/tests/test_services/test_discovery_events.pybackend/app/tests/test_services/test_newborn_origin.pybackend/app/tests/test_utils/test_dweller_ai_schemas.pybackend/app/tests/test_utils/test_places.pyfrontend/src/modules/map/components/MapMarker.vuefrontend/src/modules/map/components/MarkerDetailModal.vuefrontend/src/modules/map/components/WorldMap.vuefrontend/src/modules/map/stores/map.tsfrontend/src/modules/map/views/MapView.vuefrontend/tests/unit/components/map/MarkerDetailModal.test.tsfrontend/tests/unit/components/map/WorldMap.test.tsfrontend/tests/unit/stores/map.test.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- frontend/tests/unit/components/map/WorldMap.test.ts
- frontend/src/modules/map/views/MapView.vue
- frontend/tests/unit/components/map/MarkerDetailModal.test.ts
- backend/app/api/v1/endpoints/map.py
- backend/app/tests/test_api/test_system.py
- backend/app/models/wasteland_location.py
- backend/app/schemas/dweller_ai.py
- frontend/src/modules/map/components/MarkerDetailModal.vue
- backend/app/crud/wasteland_location.py
- frontend/src/modules/map/components/WorldMap.vue
- ROADMAP.md
- frontend/src/modules/map/stores/map.ts
- backend/app/tests/test_services/test_discovery_events.py
Comment on lines
+9
to
+35
| def test_dweller_backstory_visited_places_65_chars_rejected(): | ||
| """visited_places item > 64 chars raises pydantic.ValidationError.""" | ||
| with pytest.raises(pydantic.ValidationError): | ||
| DwellerBackstory( | ||
| bio="A test bio for backstory validation.", | ||
| origin_place="Megaton", | ||
| visited_places=["x" * 65], | ||
| ) | ||
|
|
||
|
|
||
| def test_dweller_backstory_visited_places_64_chars_valid(): | ||
| """visited_places item == 64 chars is valid (edge case).""" | ||
| result = DwellerBackstory( | ||
| bio="A test bio for backstory validation.", | ||
| origin_place="Megaton", | ||
| visited_places=["x" * 64], | ||
| ) | ||
| assert result.visited_places == ["x" * 64] | ||
|
|
||
|
|
||
| def test_extended_bio_visited_places_65_chars_rejected(): | ||
| """visited_places item > 64 chars raises pydantic.ValidationError on ExtendedBio.""" | ||
| with pytest.raises(pydantic.ValidationError): | ||
| ExtendedBio( | ||
| extended_bio="More details about the dweller.", | ||
| visited_places=["y" * 65], | ||
| ) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add return annotations to the new test functions.
Add -> None to all three test functions.
Proposed fix
-def test_dweller_backstory_visited_places_65_chars_rejected():
+def test_dweller_backstory_visited_places_65_chars_rejected() -> None:
@@
-def test_dweller_backstory_visited_places_64_chars_valid():
+def test_dweller_backstory_visited_places_64_chars_valid() -> None:
@@
-def test_extended_bio_visited_places_65_chars_rejected():
+def test_extended_bio_visited_places_65_chars_rejected() -> None:As per coding guidelines, backend/**/*.py requires: “Add type hints to new functions and prefer UUID4/Pydantic types where applicable.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_dweller_backstory_visited_places_65_chars_rejected(): | |
| """visited_places item > 64 chars raises pydantic.ValidationError.""" | |
| with pytest.raises(pydantic.ValidationError): | |
| DwellerBackstory( | |
| bio="A test bio for backstory validation.", | |
| origin_place="Megaton", | |
| visited_places=["x" * 65], | |
| ) | |
| def test_dweller_backstory_visited_places_64_chars_valid(): | |
| """visited_places item == 64 chars is valid (edge case).""" | |
| result = DwellerBackstory( | |
| bio="A test bio for backstory validation.", | |
| origin_place="Megaton", | |
| visited_places=["x" * 64], | |
| ) | |
| assert result.visited_places == ["x" * 64] | |
| def test_extended_bio_visited_places_65_chars_rejected(): | |
| """visited_places item > 64 chars raises pydantic.ValidationError on ExtendedBio.""" | |
| with pytest.raises(pydantic.ValidationError): | |
| ExtendedBio( | |
| extended_bio="More details about the dweller.", | |
| visited_places=["y" * 65], | |
| ) | |
| def test_dweller_backstory_visited_places_65_chars_rejected() -> None: | |
| """visited_places item > 64 chars raises pydantic.ValidationError.""" | |
| with pytest.raises(pydantic.ValidationError): | |
| DwellerBackstory( | |
| bio="A test bio for backstory validation.", | |
| origin_place="Megaton", | |
| visited_places=["x" * 65], | |
| ) | |
| def test_dweller_backstory_visited_places_64_chars_valid() -> None: | |
| """visited_places item == 64 chars is valid (edge case).""" | |
| result = DwellerBackstory( | |
| bio="A test bio for backstory validation.", | |
| origin_place="Megaton", | |
| visited_places=["x" * 64], | |
| ) | |
| assert result.visited_places == ["x" * 64] | |
| def test_extended_bio_visited_places_65_chars_rejected() -> None: | |
| """visited_places item > 64 chars raises pydantic.ValidationError on ExtendedBio.""" | |
| with pytest.raises(pydantic.ValidationError): | |
| ExtendedBio( | |
| extended_bio="More details about the dweller.", | |
| visited_places=["y" * 65], | |
| ) |
🤖 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 `@backend/app/tests/test_utils/test_dweller_ai_schemas.py` around lines 9 - 35,
Add a -> None return annotation to each new test function:
test_dweller_backstory_visited_places_65_chars_rejected,
test_dweller_backstory_visited_places_64_chars_valid, and
test_extended_bio_visited_places_65_chars_rejected. Do not alter their test
logic.
Source: Coding guidelines
… compat typescript 7.0.2 (native TS) removed the ts.factory API that both openapi-typescript (peerDep ^5.x) and vue-tsc depend on, breaking pnpm types:generate and pnpm run typecheck. Restore the pre-dependabot known-good state (^6.0.3) until the toolchain supports TS7.
…orts it openapi-typescript (peerDep ^5.x) and vue-tsc break on typescript 7 (native TS removed ts.factory). Prevent dependabot re-bumping past 6.x.
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
1 participant
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 by CodeRabbit