Feature/vitorlas kupa map#1037
Merged
Merged
Conversation
…ities and filter by group ID
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughBackend fixes LocationEntity id and userId handling and changes group membership filtering to load and compare user.group ids. Frontend: MapContent gains optional ChangesLocation Service ID and Map UI System
Sequence Diagram(s)sequenceDiagram
participant ComponentA
participant ComponentB
ComponentA->>ComponentB: observable interaction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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)
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 |
There was a problem hiding this comment.
Pull request overview
This PR extends the map experience by adding a fullscreen display mode in the frontend, improving map centering logic based on available markers, and adjusting backend location entity identification/group filtering for tracking-related endpoints.
Changes:
- Add a fullscreen toggle on the map page with an overlay UI (close button + Kir-Dev logo link).
- Enhance
MapContentto support customclassName/heightand center the map based on marker averages (or user location when enabled). - Update backend
LocationServiceto setLocationEntity.idbased on the user and change group filtering logic.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| frontend/src/pages/map/map.page.tsx | Adds fullscreen UI/overlay around the map and passes sizing/styling props down to MapContent. |
| frontend/src/common-components/map/MapContent.tsx | Adds optional sizing/styling props and changes map centering + user marker display behavior. |
| backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt | Adjusts location entity IDs and changes “locations by group” filtering logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+38
to
+41
| <Checkbox id="full-screen" checked={fullScreen} onCheckedChange={(checked) => setFullScreen(!!checked)} /> | ||
| <Label htmlFor="full-screen" className="cursor-pointer"> | ||
| {'Teljes képernyő'} | ||
| </Label> |
Comment on lines
+47
to
+52
| <button | ||
| onClick={() => setFullScreen(false)} | ||
| className="absolute top-4 right-4 z-50 bg-black/50 hover:bg-black/70 text-white p-2 rounded-full shadow-lg transition-colors" | ||
| aria-label="Bezárás" | ||
| > | ||
| <X size={24} /> |
Comment on lines
14
to
+16
| export default function MapPage() { | ||
| const [showUserLocation, setShowUserLocation] = useState(false) | ||
| const [fullScreen, setFullScreen] = useState(false) |
Comment on lines
+66
to
+71
| <MapContent | ||
| mapData={locationQuery.data ?? []} | ||
| showUserLocation={showUserLocation} | ||
| className={fullScreen ? 'h-screen w-screen' : ''} | ||
| height={fullScreen ? (typeof window !== 'undefined' ? window.innerHeight : 800) : 400} | ||
| /> |
Comment on lines
128
to
+132
| fun findLocationsOfGroup(groupId: Int): List<LocationEntity> { | ||
| return tokenToLocationMapping.values.filter { it.id == groupId } | ||
| return tokenToLocationMapping.values.filter { | ||
| val user = userRepository.findById(it.userId) | ||
| user.isPresent && user.get().group?.id == groupId | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.kt`:
- Around line 129-132: The current findLocationsOfGroup implementation iterates
tokenToLocationMapping.values and calls userRepository.findById per element,
causing N+1 queries and risking LazyInitializationException when accessing
user.group; fix by avoiding per-location DB fetch: either persist groupId on
LocationEntity during pushLocation/refresh and then filter
tokenToLocationMapping.values by location.groupId == groupId, or batch-fetch
users once inside a transaction (use transactionManager.transaction and
userRepository.findAllById with the collected userIds) to build a set of user
IDs in the group and then filter locations by userId in that set; update
findLocationsOfGroup accordingly and ensure pushLocation/refresh populate
location.groupId if choosing the first approach.
In `@frontend/src/common-components/map/MapContent.tsx`:
- Line 53: The Marker elements use mapDataItem.displayName as the React key
which can collide; update the key to a stable unique value (e.g., combine
latitude and longitude and displayName or add and use a dedicated id on
MapDataItemView) so markers reconcile correctly — change the Marker key in
MapContent (where Marker and mapDataItem are used) to something like
`${mapDataItem.latitude}-${mapDataItem.longitude}-${mapDataItem.displayName}` or
use mapDataItem.id if you add one to the data model.
In `@frontend/src/pages/map/map.page.tsx`:
- Line 70: The render reads window.innerHeight directly for the height prop when
fullScreen is true, which becomes stale on resize; introduce a piece of state
(e.g. fullscreenHeight via useState) initialized to window.innerHeight (with
safe typeof window guard) and replace the inline window.innerHeight usage with
that state; add a useEffect that registers a window 'resize' listener to update
fullscreenHeight on resize/orientation change and cleans up the listener on
unmount; ensure the height prop uses fullscreenHeight when fullScreen is true
and falls back to 400 otherwise.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3bc4e0e7-acab-4795-9631-98c3f72a5597
📒 Files selected for processing (3)
backend/src/main/kotlin/hu/bme/sch/cmsch/component/location/LocationService.ktfrontend/src/common-components/map/MapContent.tsxfrontend/src/pages/map/map.page.tsx
…eight handling in map page
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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
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
New Features
Improvements
Bug Fixes