Changeset comment feature + notes comment feature#88
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughComment posting now reports failures while preserving drafts, refreshes changeset discussions, and optimistically updates note discussions. OSM client requests use encoded query parameters, note parsing is refactored, and Nuxt development proxy settings are expanded. ChangesComment flow updates
Development server routing
Development workflow maintenance
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 4
🤖 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 `@nuxt.config.ts`:
- Around line 31-32: Remove the newly added trailing commas from the devServer
configuration and the additional changed entries around the corresponding
configuration block in nuxt.config.ts, following the project’s
no-trailing-commas style.
- Around line 52-56: Update the Vite server configuration in nuxt.config.ts to
remove allowedHosts: true and keep host validation enabled by using the default
empty list or an explicit allowlist. Support additional tunnel hosts through the
__VITE_ADDITIONAL_SERVER_ALLOWED_HOSTS environment variable.
In `@services/osm.ts`:
- Around line 59-78: Guard against missing comments in noteFeatureToEntity by
defaulting properties.comments to an empty array before iterating. Update the
comments access in the loop and returned OsmNote so undefined feature properties
or comments never cause iteration to throw.
- Around line 462-501: Remove the duplicate postNoteComment method in
services/osm.ts. Keep the Promise<void> implementation that posts the encoded
text query parameter, and delete or rename the later JSON-returning
implementation so only one method with this signature remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8f25c48-b8f8-4be4-b6f1-cfb7157f534b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
components/review/Discussion.vuenuxt.config.tsservices/osm.tstest/e2e/review.spec.tstest/unit/services/osm.test.ts
| function noteFeatureToEntity(feature: Feature): OsmNote { | ||
| const geometry = feature.geometry as Point; | ||
| const properties = feature.properties ?? { }; | ||
|
|
||
| for (const comment of properties.comments) { | ||
| comment.date = new Date(comment.date); | ||
| } | ||
|
|
||
| notes.push({ | ||
| id: properties.id, | ||
| status: properties.status, | ||
| lat: geometry.coordinates[1] ?? 0, | ||
| lon: geometry.coordinates[0] ?? 0, | ||
| created_at: new Date(properties.date_created), | ||
| comments: properties.comments, | ||
| }); | ||
| for (const comment of properties.comments) { | ||
| comment.date = new Date(comment.date); | ||
| } | ||
|
|
||
| return notes; | ||
| return { | ||
| id: properties.id, | ||
| status: properties.status, | ||
| lat: geometry.coordinates[1] ?? 0, | ||
| lon: geometry.coordinates[0] ?? 0, | ||
| created_at: new Date(properties.date_created), | ||
| comments: properties.comments, | ||
| }; | ||
| } | ||
|
|
||
| function notesGeoJsonToEntities(geoJson: FeatureCollection): OsmNote[] { | ||
| return geoJson.features.map(noteFeatureToEntity); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against undefined properties.comments
When feature.properties is undefined, the ?? { } fallback yields an empty object, but properties.comments is then undefined and for (const comment of undefined) throws a TypeError. Add a defensive fallback to [].
🛡️ Proposed fix
function noteFeatureToEntity(feature: Feature): OsmNote {
const geometry = feature.geometry as Point;
const properties = feature.properties ?? { };
- for (const comment of properties.comments) {
+ for (const comment of properties.comments ?? []) {
comment.date = new Date(comment.date);
}📝 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.
| function noteFeatureToEntity(feature: Feature): OsmNote { | |
| const geometry = feature.geometry as Point; | |
| const properties = feature.properties ?? { }; | |
| for (const comment of properties.comments) { | |
| comment.date = new Date(comment.date); | |
| } | |
| notes.push({ | |
| id: properties.id, | |
| status: properties.status, | |
| lat: geometry.coordinates[1] ?? 0, | |
| lon: geometry.coordinates[0] ?? 0, | |
| created_at: new Date(properties.date_created), | |
| comments: properties.comments, | |
| }); | |
| for (const comment of properties.comments) { | |
| comment.date = new Date(comment.date); | |
| } | |
| return notes; | |
| return { | |
| id: properties.id, | |
| status: properties.status, | |
| lat: geometry.coordinates[1] ?? 0, | |
| lon: geometry.coordinates[0] ?? 0, | |
| created_at: new Date(properties.date_created), | |
| comments: properties.comments, | |
| }; | |
| } | |
| function notesGeoJsonToEntities(geoJson: FeatureCollection): OsmNote[] { | |
| return geoJson.features.map(noteFeatureToEntity); | |
| function noteFeatureToEntity(feature: Feature): OsmNote { | |
| const geometry = feature.geometry as Point; | |
| const properties = feature.properties ?? { }; | |
| for (const comment of properties.comments ?? []) { | |
| comment.date = new Date(comment.date); | |
| } | |
| return { | |
| id: properties.id, | |
| status: properties.status, | |
| lat: geometry.coordinates[1] ?? 0, | |
| lon: geometry.coordinates[0] ?? 0, | |
| created_at: new Date(properties.date_created), | |
| comments: properties.comments, | |
| }; | |
| } | |
| function notesGeoJsonToEntities(geoJson: FeatureCollection): OsmNote[] { | |
| return geoJson.features.map(noteFeatureToEntity); |
🤖 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 `@services/osm.ts` around lines 59 - 78, Guard against missing comments in
noteFeatureToEntity by defaulting properties.comments to an empty array before
iterating. Update the comments access in the loop and returned OsmNote so
undefined feature properties or comments never cause iteration to throw.
Removed reminders about environment tags and added information on local dev server setup.
Removed reminders about environment tags and added information on local dev server setup.
| # types + eslint typegen that both `lint` and `typecheck` rely on. | ||
| - name: Install dependencies | ||
| run: npm ci | ||
| run: npm install |
There was a problem hiding this comment.
Just curious, why not keep ci? Wasn't it designed for this environment?
There was a problem hiding this comment.
It uses the .lock file to avoid having to re-resolve the dependencies, so it's ideal to use here, but when I dev on my Mac and this runs on Linux the versions of the dependencies aren't the same unf.
| // The OSM API reads the comment `text` from the query string, not a request | ||
| // body — a multipart body is ignored (the backend then 401s the write). | ||
| await this._post(`changeset/${changesetId}/comment?text=${encodeURIComponent(message)}`, undefined, { |
There was a problem hiding this comment.
Did you double-check Claude here? The docs suggest that the post body is correct (and there is no 401). Maybe the backend proxy is tripping something up?
https://wiki.openstreetmap.org/wiki/API_v0.6#Comment:_POST_/api/0.6/changeset/#id/comment
There was a problem hiding this comment.
I'll double-check this, but it works as written right now
There was a problem hiding this comment.
Comment was wrong, I removed it, code is right tho
|
Is there anything pending for this to be merged? @jeffmaki |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nuxt.config.ts (1)
40-45: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRewrite the dev-proxy mounts to strip the public prefix.
VITE_*_URLpoints clients at/api/v1/,/new-api/v1/,/tdei/v1/,/tdei-user/v1/, and/osm/, but these proxy targets still need to map to upstream paths like/api/v1/and/. Add explicit path rewriting (or equivalent proxy config) for all five prefixes so requests don’t land on duplicated paths such as/api/api/v1/...or/api/tdei/v1/....🤖 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 `@nuxt.config.ts` around lines 40 - 45, Update the devProxy entries for /api, /new-api, /osm, /tdei, and /tdei-user to strip each public mount prefix before forwarding, while preserving the upstream base paths (/api/v1/ for API services and / for OSM). Ensure requests do not produce duplicated prefixes such as /api/api/v1 or /api/tdei/v1.
🤖 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 @.github/workflows/ci.yml:
- Line 31: Replace the npm install command in the CI workflow with npm ci to
enforce the committed package-lock.json and fail on manifest or lockfile
divergence. Keep the existing dependency installation step otherwise unchanged.
In `@nuxt.config.ts`:
- Around line 54-58: Update the allowedHosts configuration in nuxt.config.ts to
remove the broad '.local' and '.internal' entries, retaining only the exact
organization and tunnel hosts required for the dev server. Direct developers to
use __VITE_ADDITIONAL_SERVER_ALLOWED_HOSTS for any per-developer host additions.
In `@README.md`:
- Around line 13-16: Update the README proxy documentation to refer to the
nitro.devProxy configuration instead of a devServer section, clarify that the
commented lines 47-49 are local proxy targets, and change “setup” to “set up” or
“is set up” as grammatically appropriate.
---
Outside diff comments:
In `@nuxt.config.ts`:
- Around line 40-45: Update the devProxy entries for /api, /new-api, /osm,
/tdei, and /tdei-user to strip each public mount prefix before forwarding, while
preserving the upstream base paths (/api/v1/ for API services and / for OSM).
Ensure requests do not produce duplicated prefixes such as /api/api/v1 or
/api/tdei/v1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9171050-0165-4f59-9fb6-0bb562dbc845
📒 Files selected for processing (9)
.env.example.github/workflows/ci.yml.gitignoreREADME.mdcomponents/review/Discussion.vuenuxt.config.tsservices/osm.tstest/e2e/review.spec.tstest/unit/services/osm.test.ts
💤 Files with no reviewable changes (1)
- services/osm.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/unit/services/osm.test.ts
- test/e2e/review.spec.ts
- components/review/Discussion.vue
| # types + eslint typegen that both `lint` and `typecheck` rely on. | ||
| - name: Install dependencies | ||
| run: npm ci | ||
| run: npm install |
There was a problem hiding this comment.
Keep CI lockfile-deterministic.
npm install weakens the repository’s existing CI contract: unlike npm ci, it can reconcile/update the lockfile rather than failing when manifests and the lockfile diverge. That allows CI to validate a dependency tree different from the committed one; restore npm ci, or document and verify the platform-specific dependency rationale separately.
#!/usr/bin/env bash
set -euo pipefail
test -f package-lock.json
npm install --dry-run --ignore-scripts
git diff --exit-code -- package-lock.json🤖 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 @.github/workflows/ci.yml at line 31, Replace the npm install command in the
CI workflow with npm ci to enforce the committed package-lock.json and fail on
manifest or lockfile divergence. Keep the existing dependency installation step
otherwise unchanged.
|
Yes, I want to address that latest round of Rabbit comments please. I'll merge after that. |
3 participants
AZDO #3967 and #3966
Summary
components/review/Discussion.vue, including:ChatMessagemapping into a helper used byrefreshNote()messagesfromosmClient.getChangesetComments(relying on it to return an empty array when appropriate)services/osm.ts:text=...) instead of multipartFormDatafor changeset commentsOsmApiClient.postNoteComment(...)to post note comments via the JSON endpoint (also using a query param) and return the updated notegetChangesetComments(including date typing and empty-array behavior)text=URL encoding for both changeset and note endpointsdevServer.hostset to0.0.0.0nitro.devProxyinto object-based route rules (withchangeOrigin) and expanded proxied SPA backends (adding/new-api,/osm,/tdei,/tdei-user, while removing/workspaces)server.allowedHostspatterns.env.exampleandREADME.mdto reflect the new devProxy routing approachnpm citonpm install.gitignoreto ignore Claude-related paths