Repository navigation
Make the start point search ignore case - #546
Conversation
Fixes #535. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MpWbDtJZDUXbNzqBGZthUm
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
lmonteirotech
left a comment
lmonteirotech
left a comment
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED (one small point, on the head 8a4432e; posted as a comment because the PR is on the same account as this review)
Independent review against main f6a59fe (the head is based on current main).
Fine
- Scope is minimal: one filter line in
start_point, a smallentry_matcheshelper, one changelog line.super::ui::matchesalready expects a lowercased query, so lowercasing and trimming in the helper is the right fix, and the empty case still shows every entry. - No new strings and no locale impact. The changelog line sits under Fixed and cites #535. Provenance is clean (own change), and the commit message has nothing forbidden.
- The test covers "Laranjeiras", " LARANJEIRAS ", lowercase, empty and a non-match. It would fail on the old behaviour for the capitalised query.
To change
- #527 (merged into main minutes ago) moved every launcher test out of
src/, and its description says that after itomsi-apphas no test left insrc/. This PR adds an inline#[cfg(test)] mod teststocrates/omsi-app/src/launcher/start.rs, which goes against that convention. Please move the test tocrates/omsi-app/tests/app/launcher/start.rs, which already doesuse openomsi_game::testing::launcher::start::*;. That needsentry_matchesto bepub(the module is re-exported throughtesting::launcher). Visibility is the only change outside the test.
Not verified
- CI was still running on this head when I looked (only
provenancehad finished, green). The test has not been run by anyone yet. After the move, the merge rule still needs CI 7/7 on the new head.
Generated by Claude Code
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a4432e917
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #[cfg(test)] | ||
| mod tests { | ||
| use super::entry_matches; |
There was a problem hiding this comment.
The new inline test module violates the repository's explicit testing convention: CONTRIBUTING.md, made mandatory by AGENTS.md, requires tests under each crate's tests/ directory and access to internals through testing behind test-support. This module already has tests in crates/omsi-app/tests/app/launcher/start.rs and a corresponding testing::launcher::start export. Add the regression there and expose the helper through the established testing surface rather than reintroducing tests under src/.
AGENTS.md reference: AGENTS.md:L43-L43
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4b93bd1: the test now lives in crates/omsi-app/tests/app/launcher/start.rs, entry_matches is pub (reached through testing::launcher::start), and the inline mod tests is gone.
Generated by Claude Code
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MpWbDtJZDUXbNzqBGZthUm
lmonteirotech
left a comment
lmonteirotech
left a comment
There was a problem hiding this comment.
Verdict: APPROVED on the head 4b93bd1 (posted as a comment because the PR is on the same account as this review)
Re-reviewed the exact head against main f6a59fe (still the base).
- The point from my earlier review is fixed: the inline
mod testsis gone fromsrc/, the teststart_point_search_ignores_casenow lives incrates/omsi-app/tests/app/launcher/start.rs(which already importstesting::launcher::start::*), andentry_matchesispub, the only visibility change, as #527 did. - Scope stays minimal: one filter line, the helper, the test and one Fixed line in the changelog citing #535. No new strings, no locale impact. Provenance is clean and neither commit message has anything forbidden.
- The test covers a capitalised, upper-case padded, lowercase, empty and non-matching query, and fails on the old behaviour for "Laranjeiras".
Condition for merging: CI 7/7 on this exact head. I did not run the tests here, and I did not wait for CI.
Generated by Claude Code
Requested by Leo · project thread
Fixes #535.
What
The start point picker (Maps → Start → Choose the start point) passed the typed query as is to
matches, which only lowercases the entry name. A query with a capital letter ("Laranjeiras") found nothing. The picker now lowercases and trims the query through a smallentry_matcheshelper.Tests
New unit test
start_point_search_ignores_case(capitalised, upper case, padded, empty and non-matching queries). Written against the old behaviour it fails for "Laranjeiras". Not run locally (the container lacks system libs for the build); CI runs it.Scope
No new strings, no behaviour change besides the search. The map preview in this picker showing only entry points (no streets) was reported as probably performance on the tester's laptop and is deliberately not touched here.
Provenance
Own change, no third-party code.
Generated by Claude Code