Conversation
Contributor
Author
…oute elevation average
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes are well-covered by new unit tests and the production code edits are small, targeted, and consistent with the validated behaviors.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
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.
AI disclaimer
Written with Claude (Cowork, Claude Fable 5.1), driven by @alisa911. Summary of the requests it worked from, in order:
After a review of the previous PR found a test that pinned broken behaviour, asked to go through every unit test again and check whether each expectation is what the app should do, not what the code does. All 31 files were checked against the server (osmandapp/tools) and Android (osmandapp/android). One test was found to pin a real divergence - the web escapes both
/and:in a favorite group file name while Android escapes only the first one it finds - and a few more doubtful cases; they went into KNOWN_ISSUES.md (not part of this PR), and the rule "expect what the app should do, not what it does" went into the add-test skill.Cover the routing cache of the track editor.
effectControlRouterRequestslet a seventh request through when six were in flight (>instead of>=againstMAX_STARTED_ROUTER_JOBS); fixed after asking. Six tests inrouting/01-routing-cache.test.js: a segment is cached by its points and profile, a failed request is not repeated, a routed segment goes into the track once, undo restores the geometry from the cache, the debouncer keeps the last call.Cover the distances of a track.
getTrackPointsshifteddistanceTotalof the second and following<trk>of a file in place, and it is called on every redraw of the graph, so the distance axis grew on each call. The author asked to re-check that the bug is real before fixing; the chain was verified end to end (WebGpxParser.getTracksreturns one entry per<trk>,addDistancecounts each from zero,getAllPointsreturns references,GpxGraphProviderreadsdistanceTotal). Fixed by shifting copies; seven tests intracks/14-track-distance.test.js. The first version of the test compared mutated points with themselves and passed - caught and rewritten to snapshot the numbers first.Cover deleting and reordering the points of a track. A point dragged to the first place in the Points tab kept the geometry of its old segment, so
validateRoutePointsrejected the track and the analysis and the save ignored the reorder; fixed ininsertPoint. The author reminded about the add-test skill: the inline contexts of the new tests were moved intofixtures/tracks.js(createEditorCtx,createEditorPoint,createRoutedPoints). Five tests intracks/15-track-points.test.js.Cover the conversion of a navigation route into a track. No bugs; three tests in
navigation/00-route-to-track.test.js(segments cut at the route points, a via point snapped to the line, alternatives left out, elevation borrowed at the ends of the line). Two doubts noted without a fix: a round trip whose finish is on the start collapses both into one map entry, and the point shared by two segments was counted twice in the average elevation - the latter was fixed on review, see 8.Cover the cloud changes and trash.
deleteVersionsFromMenukept the header of a month whose files were all deleted as long as any later month still had a file; the author asked to re-check, the rendering inCloudChanges/CloudTrashwas confirmed to draw headers as they come; fixed. Five tests insettings/00-cloud-changes.test.js.Cover the url helpers, the file storages and the matched objects of the spatial search. No bugs; seven tests in
util/07-url-and-storage.test.jsandsearch/03-matched-objects.test.js. A matched amenity without coordinates would be listed as a link that does nothing - not pinned by a test, since the server builds matched objects from map objects with coordinates.Four review comments applied:
deleteFilenow goes throughdeleteVersionsFromMenulikerestoreFile(the test had pinned the leftover month header again);isLastof the row that became the last of its month is recomputed, so no divider is drawn before the next header; the point shared by two segments of a route counts once increateAnalysisFromRoute(the test expects 115 with the arithmetic written down); the skill names the repositories instead of local paths.No commits were made by the agent; every commit was reviewed and made by the author. The 406 unit tests were run by the agent, the selenium suite by the author.