fix: photos are optional - #390
Conversation
|
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:
📝 WalkthroughWalkthroughThe suggested-point flow consumes object-based categories and backend-provided photo constraints. It compresses JPEG uploads, reports errors inline, shows success toasts, centers the toaster within the map, and normalizes notification attachments. ChangesSuggested Point Flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SuggestNewPointButton
participant compressImageToJpeg
participant Backend
participant AppToaster
User->>SuggestNewPointButton: select and submit suggestion
SuggestNewPointButton->>compressImageToJpeg: compress oversized photo
compressImageToJpeg-->>SuggestNewPointButton: return JPEG file or processing error
SuggestNewPointButton->>Backend: send validated suggestion
Backend-->>SuggestNewPointButton: return success or backend error
SuggestNewPointButton->>AppToaster: show success toast
SuggestNewPointButton-->>User: show inline validation or submission error
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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 `@frontend/src/components/Map/components/SuggestNewPointButton.jsx`:
- Around line 106-114: Update the location schema handling in the
SuggestNewPointButton component so an existing globalThis.LOCATION_SCHEMA
without a photo property supplies a safe default photo object before
destructuring allowed_mime_types and max_size_bytes. Preserve the existing
defaults for a completely missing schema and ensure rendering does not throw
when photo is absent.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 63ac1c5b-ed0a-4c6a-a1f1-3cbe47de6709
📒 Files selected for processing (11)
frontend/src/components/Map/components/SuggestNewPointButton.jsxfrontend/src/components/common/AppToaster.jsxfrontend/src/locales/en/map.jsonfrontend/src/locales/pl/map.jsonfrontend/src/locales/ua/map.jsonfrontend/src/utils/imageCompression.jsfrontend/tests/Map/components/SuggestNewPoint.test.jsxfrontend/tests/utils/testConstants.jsgoodmap/core_api.pygoodmap/goodmap.pytests/unit_tests/test_core_api.py
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)
frontend/src/components/Map/components/SuggestNewPointButton.jsx (2)
168-182: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAlign the photo limit with the multipart request limit.
The frontend accepts processed photos up to the configured 5 MB limit, but the backend default sets
MAX_CONTENT_LENGTHto 100 KB. Photos can pass this check and still be rejected before/api/suggest-new-pointruns. Increase the request limit to include the configured photo size and multipart overhead, or expose and enforce the smaller effective limit inLOCATION_SCHEMA. (raw.githubusercontent.com)🤖 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 `@frontend/src/components/Map/components/SuggestNewPointButton.jsx` around lines 168 - 182, The frontend photo-size validation is inconsistent with the backend multipart request limit, allowing uploads that are rejected before suggest-new-point processing. Update the backend request-size configuration near MAX_CONTENT_LENGTH to accommodate the configured maxPhotoSizeBytes plus multipart overhead, or expose and enforce the smaller effective limit through LOCATION_SCHEMA so both validation paths use the same limit.
156-186: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBlock submission until photo processing completes.
handlePhotoUploadawaits compression, but the submit button remains enabled. If the user submits during that wait,photostill contains the previous file ornull. The request can omit the selected photo or send the wrong photo. A lateacceptPhotocall can also repopulate state after the success reset and affect the next suggestion. Track a processing state and upload generation. Disable or await submission, and ignore stale results. (raw.githubusercontent.com)🤖 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 `@frontend/src/components/Map/components/SuggestNewPointButton.jsx` around lines 156 - 186, Update handlePhotoUpload and the submit flow to track photo-processing state and an upload generation/token. Disable or defer submission while processing, clear or invalidate prior photo state when a new file is selected, and apply compression results only if they belong to the latest upload so stale acceptPhoto calls cannot repopulate state after reset.
🤖 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 `@docs/quickstart.rst`:
- Around line 189-193: Update the photo_attachment_config size limit and its
accompanying documentation/backend error text to use one consistent unit: either
retain 5 * 1024 * 1024 and label it 5 MiB everywhere, or use 5_000_000 bytes and
label it 5 MB everywhere. Apply the same correction to the additionally
referenced occurrence.
---
Outside diff comments:
In `@frontend/src/components/Map/components/SuggestNewPointButton.jsx`:
- Around line 168-182: The frontend photo-size validation is inconsistent with
the backend multipart request limit, allowing uploads that are rejected before
suggest-new-point processing. Update the backend request-size configuration near
MAX_CONTENT_LENGTH to accommodate the configured maxPhotoSizeBytes plus
multipart overhead, or expose and enforce the smaller effective limit through
LOCATION_SCHEMA so both validation paths use the same limit.
- Around line 156-186: Update handlePhotoUpload and the submit flow to track
photo-processing state and an upload generation/token. Disable or defer
submission while processing, clear or invalidate prior photo state when a new
file is selected, and apply compression results only if they belong to the
latest upload so stale acceptPhoto calls cannot repopulate state after reset.
🪄 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: CHILL
Plan: Pro Plus
Run ID: d33f8581-0119-4678-9279-b1e70c9b144a
📒 Files selected for processing (11)
docs/quickstart.rstfrontend/src/components/Map/components/SuggestNewPointButton.jsxfrontend/src/components/common/AppToaster.jsxfrontend/src/locales/en/map.jsonfrontend/src/locales/pl/map.jsonfrontend/src/locales/ua/map.jsonfrontend/tests/Map/components/SuggestNewPoint.test.jsxfrontend/tests/utils/testConstants.jsgoodmap/core_api.pytests/unit_tests/test_core_api.pytests/unit_tests/test_goodmap.py
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/src/locales/ua/map.json
- frontend/src/locales/en/map.json
- frontend/src/locales/pl/map.json
- frontend/tests/utils/testConstants.js
- frontend/src/components/common/AppToaster.jsx
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 `@frontend/tests/utils/imageCompression.test.js`:
- Around line 38-42: Update the toBlob mock setup in the image compression tests
to record its third quality argument for every call. Extend retry assertions to
verify that each attempt uses a lower quality than the previous one, and ensure
the compression retry logic clamps the next quality to MIN_QUALITY before
encoding.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 2c195d60-5514-4c53-80f4-da0f5972d20d
📒 Files selected for processing (12)
docs/quickstart.rste2e-tests/tests/basic/test_suggest_new_point.pyfrontend/src/components/Map/components/SuggestNewPointButton.jsxfrontend/src/locales/en/map.jsonfrontend/src/locales/pl/map.jsonfrontend/src/locales/ua/map.jsonfrontend/tests/Map/components/SuggestNewPoint.test.jsxfrontend/tests/utils/imageCompression.test.jsfrontend/tests/utils/testConstants.jsgoodmap/core_api.pygoodmap/goodmap.pytests/unit_tests/test_core_api.py
🚧 Files skipped from review as they are similar to previous changes (10)
- frontend/src/locales/en/map.json
- goodmap/goodmap.py
- goodmap/core_api.py
- frontend/src/locales/pl/map.json
- frontend/src/locales/ua/map.json
- frontend/tests/utils/testConstants.js
- docs/quickstart.rst
- frontend/tests/Map/components/SuggestNewPoint.test.jsx
- tests/unit_tests/test_core_api.py
- frontend/src/components/Map/components/SuggestNewPointButton.jsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/Map/components/SuggestNewPointButton.jsx (1)
474-477: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDisable submit while a photo is still compressing.
handlePhotoUploadsetsisCompressingPhotototruefor the duration ofcompressImageToJpeg(Lines 216-229), but the Submit button at Line 475 has nodisabledbinding to that state, andhandleConfirmNewPointdoes not check it either. If the user picks an oversized photo and clicks Submit before compression resolves, the point is created without the photo, sincephotois stillnullor a stale value at submit time. Nothing tells the user that their photo was dropped.Disable Submit while
isCompressingPhotoistrue, mirroring the existing disable on the upload input/button.🛡️ Proposed fix
- <Button type="submit" variant="contained" color="primary"> + <Button + type="submit" + variant="contained" + color="primary" + disabled={isCompressingPhoto} + > {t('submit')} </Button>🤖 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 `@frontend/src/components/Map/components/SuggestNewPointButton.jsx` around lines 474 - 477, Update the submit Button in the DialogActions to bind its disabled state to isCompressingPhoto, matching the existing upload control behavior. Ensure submission remains disabled only while handlePhotoUpload is compressing the selected photo, allowing normal submission otherwise.
🧹 Nitpick comments (1)
frontend/tests/Map/components/SuggestNewPoint.test.jsx (1)
391-415: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the mocked
scrollToeven if the assertion fails.
HTMLElement.prototype.scrollTois mocked at Line 394 and only cleaned up at Line 414, after thewaitForassertions. If the assertion at Lines 409-412 fails (for example, a real regression of the scroll-to-top behavior), thedeleteon Line 414 never runs, and the mockedscrollToleaks into subsequent tests in this file.Move the restoration into
afterEachor wrap the test body intry/finally.♻️ Proposed fix
- axios.post.mockResolvedValue({}); - mockGeolocationSuccess(); - - renderWithProvider(<SuggestNewPointButton />); - await openDialog(); - - submitForm(); - - await waitFor(() => { - expect(screen.getByRole('alert')).toHaveTextContent(ERROR_MESSAGES.REQUIRED_FIELDS); - expect(scrollTo).toHaveBeenCalledWith({ top: 0, behavior: 'smooth' }); - }); - - delete HTMLElement.prototype.scrollTo; + try { + axios.post.mockResolvedValue({}); + mockGeolocationSuccess(); + + renderWithProvider(<SuggestNewPointButton />); + await openDialog(); + + submitForm(); + + await waitFor(() => { + expect(screen.getByRole('alert')).toHaveTextContent(ERROR_MESSAGES.REQUIRED_FIELDS); + expect(scrollTo).toHaveBeenCalledWith({ top: 0, behavior: 'smooth' }); + }); + } finally { + delete HTMLElement.prototype.scrollTo; + }🤖 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 `@frontend/tests/Map/components/SuggestNewPoint.test.jsx` around lines 391 - 415, Ensure the HTMLElement.prototype.scrollTo mock created in the “scrolls the dialog content back to top when a notice appears” test is always restored, including when assertions fail. Move cleanup into an appropriate afterEach hook or use try/finally, preserving the existing test behavior and preventing the mock from leaking into subsequent tests.
🤖 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.
Outside diff comments:
In `@frontend/src/components/Map/components/SuggestNewPointButton.jsx`:
- Around line 474-477: Update the submit Button in the DialogActions to bind its
disabled state to isCompressingPhoto, matching the existing upload control
behavior. Ensure submission remains disabled only while handlePhotoUpload is
compressing the selected photo, allowing normal submission otherwise.
---
Nitpick comments:
In `@frontend/tests/Map/components/SuggestNewPoint.test.jsx`:
- Around line 391-415: Ensure the HTMLElement.prototype.scrollTo mock created in
the “scrolls the dialog content back to top when a notice appears” test is
always restored, including when assertions fail. Move cleanup into an
appropriate afterEach hook or use try/finally, preserving the existing test
behavior and preventing the mock from leaking into subsequent tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ff69ff2-9a63-42c2-953d-c1792d016666
📒 Files selected for processing (9)
docs/quickstart.rste2e-tests/tests/basic/test_suggest_new_point.pyfrontend/src/components/Map/components/SuggestNewPointButton.jsxfrontend/src/components/common/AppToaster.jsxfrontend/src/locales/en/map.jsonfrontend/src/locales/pl/map.jsonfrontend/src/locales/ua/map.jsonfrontend/tests/Map/components/SuggestNewPoint.test.jsxfrontend/tests/utils/testConstants.js
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/locales/ua/map.json
- docs/quickstart.rst
- frontend/tests/utils/testConstants.js
- frontend/src/components/common/AppToaster.jsx
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/unit_tests/test_goodmap.py (1)
172-176: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExercise the YAML configuration path.
The docstring claims coverage for
ATTACHMENT:in deployment YAML, but this test constructsGoodmapConfigdirectly. A regression inGoodmapConfig.parse_yaml()could leave deployments on the default while this test still passes. Build the config throughGoodmapConfig.parse_yaml()with a temporary YAML file, or add a dedicated configuration test.🤖 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 `@tests/unit_tests/test_goodmap.py` around lines 172 - 176, Update test_index_route_location_schema_respects_attachment_config_override to load its GoodmapConfig through GoodmapConfig.parse_yaml() using a temporary YAML file containing the ATTACHMENT override, rather than constructing the config directly. Keep the existing assertions that verify the attachment constraints are applied.
🤖 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 `@frontend/src/components/common/AppToaster.jsx`:
- Around line 78-85: Update the containerStyle in AppToaster to remove the
zero-height centering hack and use a fixed vertical offset for mapBounds-based
placement instead of top: '50%' with translateY('-50%'). Add or update coverage
for the rendered toast to verify its position aligns with the map midpoint,
while preserving the existing measured horizontal placement.
In `@goodmap/config.py`:
- Around line 20-23: Update the default Flask request limit configured in
goodmap.py so it exceeds the configured attachment.max_size plus multipart
overhead, including ATTACHMENT configurations of 8 MiB or more; preserve
explicit user-configured limits. Add an integration test covering successful
upload of an allowed photo larger than 100 KiB reaching create_attachment.
In `@tests/unit_tests/test_goodmap.py`:
- Around line 186-190: Extend the upload tests around the ATTACHMENT
configuration and photo/suggest endpoint to cover multipart payloads below and
above the configured 8 MiB limit. Ensure the effective request limit is derived
from config.attachment.max_size with multipart overhead, or apply a
route/body-specific limit so oversized uploads are rejected before buffering;
preserve the existing schema assertion.
---
Nitpick comments:
In `@tests/unit_tests/test_goodmap.py`:
- Around line 172-176: Update
test_index_route_location_schema_respects_attachment_config_override to load its
GoodmapConfig through GoodmapConfig.parse_yaml() using a temporary YAML file
containing the ATTACHMENT override, rather than constructing the config
directly. Keep the existing assertions that verify the attachment constraints
are applied.
🪄 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: CHILL
Plan: Pro Plus
Run ID: c0fcfd06-1b2d-4503-a535-a2e4f2a64fa5
📒 Files selected for processing (7)
docs/quickstart.rstfrontend/src/components/Map/components/SuggestNewPointButton.jsxfrontend/src/components/common/AppToaster.jsxfrontend/tests/Map/components/SuggestNewPoint.test.jsxgoodmap/config.pygoodmap/goodmap.pytests/unit_tests/test_goodmap.py
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/quickstart.rst
- goodmap/goodmap.py
- frontend/src/components/Map/components/SuggestNewPointButton.jsx
| containerStyle={{ | ||
| zIndex: 99999999, | ||
| top: '50%', | ||
| bottom: 'auto', | ||
| transform: 'translateY(-50%)', | ||
| ...(mapBounds | ||
| ? { left: mapBounds.left, width: mapBounds.width, right: 'auto' } | ||
| : {}), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm the installed version and inspect the library positioning implementation.
fd -HI -t f -g 'package.json' -g 'package-lock.json' -g 'yarn.lock' -g 'pnpm-lock.yaml' frontend \
-x rg -n -C 2 '"react-hot-toast"|react-hot-toast' {}
curl -fsSL \
https://raw.githubusercontent.com/timolins/react-hot-toast/v2.4.1/src/components/toaster.tsx \
| rg -n -C 3 "position: 'absolute'|getPositionStyle|containerStyle"Repository: Problematy/goodmap
Length of output: 1484
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant AppToaster markup/styles and package lock entries for react-hot-toast.
fd -HI -t f AppToaster.jsx frontend -x sh -c 'echo "=== $1 ==="; sed -n "1,160p" "$1"' sh {}
printf '\n=== react-hot-toast lock entries ===\n'
fd -HI -t f package-lock.json frontend -x sed -n '/"react-hot-toast"/,/}/p' {}
printf '\n=== local react-hot-toast source if mounted ===\n'
fd -HI -t f toaster.tsx frontend node_modules . \
-x sh -c 'echo "=== $1 ==="; sed -n "1,150p" "$1"' sh {}
# Static CSS check: percentage transform in a zero-height absolute wrapper.
python3 - <<'PY'
from pathlib import Path
p = next(Path('frontend').glob('**/AppToaster.jsx'), None)
print(f'AppToaster found: {p}')
text = p.read_text() if p else ''
checks = {
'has top 50%': "top: '50%'" in text,
'has bottom auto': "bottom: 'auto'" in text,
'has translateY -50%': "translateY(-50%)" in text,
'has Toaster containerStyle spread': 'containerStyle={{' in text,
}
for k,v in checks.items(): print(f'{k}: {v}')
PYRepository: Problematy/goodmap
Length of output: 5546
🌐 Web query:
MDN CSS transform translateY percentage parent element size height zero
💡 Result:
When using percentage values in CSS transform functions such as translateY, the percentage refers to the size of the element itself—specifically its own reference box—rather than the size of its parent element [1][2][3]. For a vertical translation like translateY(50%), the element is moved by 50% of its own height [2]. This behavior is distinct from layout properties such as top, margin, or padding, which are typically relative to the dimensions of the parent or containing block [4][5]. Key technical details: - Reference Box: The percentage is relative to the element's bounding box [3]. For most elements, this is the border box [3]. - Parent Element Size: If you require a translation based on the parent element's height rather than the element's own height, you can use container query units (e.g., cqh) if supported, or rely on absolute positioning combined with top/bottom properties, which are relative to the containing block [5]. - SVG Elements: For SVG elements, the behavior may differ, as they often use the referenced element's bounding box or the viewport depending on the context [6][5]. In summary, transform percentages are self-referential regarding the element being transformed [4]. If the element has a height of zero, a percentage-based translateY will also result in zero translation [1][7][8].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Values/transform-function/translateY
- 2: https://css-tricks.com/almanac/functions/t/translatey/
- 3: https://stackoverflow.com/questions/34551381/is-the-css3-transform-translate-percentage-values-relative-to-its-width-and-or-h
- 4: https://stackoverflow.com/questions/46785836/why-percent-values-in-css-translates-are-relative-to-the-element-itself
- 5: https://stackoverflow.com/questions/11160227/translate-x-and-y-percentage-values-based-on-elements-height-and-width
- 6: https://www.w3.org/TR/css-transforms/
- 7: https://www.w3.org/TR/css-transforms-1/
- 8: https://drafts.csswg.org/css-transforms/
Remove the zero-height centering hack from AppToaster.
react-hot-toast renders its toasts through the single Toaster container, and its toast wrappers are absolutely positioned within that container. With top: '50%', bottom: 'auto', and no measured container height, translateY(-50%) has no vertical offset from that zero-height reference. Use a fixed vertical offset for the measured map-based placement instead, and add coverage that checks the rendered toast position against the map midpoint.
🤖 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 `@frontend/src/components/common/AppToaster.jsx` around lines 78 - 85, Update
the containerStyle in AppToaster to remove the zero-height centering hack and
use a fixed vertical offset for mapBounds-based placement instead of top: '50%'
with translateY('-50%'). Add or update coverage for the rendered toast to verify
its position aligns with the map midpoint, while preserving the existing
measured horizontal placement.
|
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation