Conversation
Closed
Reviewer's GuideAdds official support for Typesense v30.1 by updating version-tagged tests, CI workflows, and Docker configuration, and broadens the Analytics.upsert_analytics_rule return type to handle an additional server response schema. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The
@taglists for Typesense versions are repeated verbatim across many tests; consider extracting these into shared module attributes or helper macros per major version to reduce duplication and make future version bumps less error‑prone. - The new
ci_v30.1.ymlworkflow is both reusable (workflow_call) and directly triggered onpull_request, whilellm.ymlalso calls it viauses; this will cause duplicate runs on PRs, so you may want to remove the directpull_requesttrigger from the reusable workflow and keep it only in the caller. - In
ci_v30.1.yml, the cache save step referencessteps.beam.outputs.*, but thesetup-beamstep has noid: beam; either addid: beamto that step or adjust the cache key to avoid referring to a non‑existent step output.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `@tag` lists for Typesense versions are repeated verbatim across many tests; consider extracting these into shared module attributes or helper macros per major version to reduce duplication and make future version bumps less error‑prone.
- The new `ci_v30.1.yml` workflow is both reusable (`workflow_call`) and directly triggered on `pull_request`, while `llm.yml` also calls it via `uses`; this will cause duplicate runs on PRs, so you may want to remove the direct `pull_request` trigger from the reusable workflow and keep it only in the caller.
- In `ci_v30.1.yml`, the cache save step references `steps.beam.outputs.*`, but the `setup-beam` step has no `id: beam`; either add `id: beam` to that step or adjust the cache key to avoid referring to a non‑existent step output.
## Individual Comments
### Comment 1
<location path=".github/workflows/ci_v30.1.yml" line_range="24-25" />
<code_context>
+ # - [skip actions]
+ # - [actions skip]
+
+ test:
+ if: ${{ (github.event_name == 'push' || github.event_name == 'pull_request') && github.repository == 'jaeyson/open_api_typesense' }}
+ runs-on: ubuntu-latest
+ environment: review
</code_context>
<issue_to_address>
**issue (bug_risk):** The `test` job will never run when this workflow is invoked via `workflow_call` (e.g. from `llm.yml`).
Because the `if` only allows `push` or `pull_request`, this job is always skipped when invoked via `workflow_call`, so `llm.yml` does not actually run these CI steps.
If this job should also run when used as a reusable workflow, either remove the `if` altogether or expand it to include `workflow_call` (e.g. by allowing any event except `schedule`, or explicitly checking for `workflow_call`).
</issue_to_address>
### Comment 2
<location path=".github/workflows/ci_v30.1.yml" line_range="46-51" />
<code_context>
- lint: true
+ lint: false
services:
typesense:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Typesense is defined both as a GitHub Actions service and started manually with `docker run`, which is redundant and may conflict.
You both define a `services.typesense` container and start another Typesense via `docker run` on port 8108. Only the manually started instance is actually waited on, so the service container is unused and can cause port/name conflicts. Please choose a single approach: either rely on the `services.typesense` container and drop the manual `docker run`, or remove the `services` block and use only the manually started container.
```suggestion
steps:
- name: Checkout repo
```
</issue_to_address>
### Comment 3
<location path=".github/workflows/ci_v30.1.yml" line_range="155-164" />
<code_context>
+ run: mix coveralls.github
+ if: ${{ matrix.lint && github.event_name == 'push' && github.ref == 'refs/heads/main' }}
+
+ - name: Save cache
+ id: cache_save
+ uses: actions/cache/save@668228422ae6a00e4ad889ee87cd7109ec5666a7
+ if: ${{ matrix.lint }}
+ with:
+ path: |
+ deps
+ _build
+ priv/plts
+ key: |
+ ${{ runner.os }}-${{ steps.beam.outputs.otp-version }}-${{ steps.beam.outputs.elixir-version }}-${{ hashFiles('**/mix.lock') }}
</code_context>
<issue_to_address>
**issue (performance):** The cache save key does not match the restore key and references a non-existent `beam` step.
The restore key uses `matrix.typesense`, `matrix.otp`, and `matrix.elixir`, while the save key uses `${{ steps.beam.outputs.otp-version }}` and `${{ steps.beam.outputs.elixir-version }}`. Since there is no step with `id: beam`, those outputs will be empty and the save key will never match the restore key, so the cache won't be reused.
To fix this, either:
- Give the `erlef/setup-beam` step `id: beam` and use its outputs in both restore and save keys, or
- Use the same `matrix.*`-based key in both steps.
</issue_to_address>
### Comment 4
<location path=".github/workflows/ci_v30.0.yml" line_range="43" />
<code_context>
otp: '28'
elixir: '1.18'
- lint: true
+ lint: false
services:
</code_context>
<issue_to_address>
**issue (testing):** Disabling `lint` here means this workflow no longer runs any lint/static-analysis steps in any matrix variant.
This change disables the only matrix entry that previously ran the lint/static-analysis steps (`deps.unlock --check-unused`, `hex.audit`, `deps.audit`, Credo, format, Dialyzer, Coveralls`), so `ci_v30.0.yml` will no longer run any linting. If you still want lint/static analysis in this workflow, keep this entry as `lint: true` or designate another matrix entry for linting instead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
TIP This summary will be updated as you push new changes. Give us feedback
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- open_api_typesense/lib/open_api_typesense/operations/analytics.ex is usually changed with: open_api_typesense/lib/open_api_typesense/operations/debug.ex, open_api_typesense/lib/open_api_typesense/operations/collections.ex, open_api_typesense/lib/open_api_typesense/operations/operations.ex, open_api_typesense/lib/open_api_typesense/operations/synonyms.ex, open_api_typesense/lib/open_api_typesense/operations/override.ex, open_api_typesense/lib/open_api_typesense/operations/presets.ex, open_api_typesense/lib/open_api_typesense/operations/curation.ex, open_api_typesense/lib/open_api_typesense/operations/conversations.ex
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
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
1 participant
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.
Closes #44
Summary by Sourcery
Add support for Typesense server version 30.1 across tests, CI, and local development, including minor API typing adjustment for analytics rules.
Enhancements:
upsert_analytics_rule/3to handle both analytics rule responses and analytics rule schema responses.Build:
ci_v30.1.ymlworkflow running tests and linting against Typesense 30.1 across multiple Elixir/OTP versions.CI:
30.1tag and that many tests now run for Typesense 30.1 alongside existing supported versions.Deployment:
Tests:
30.1version tag so they execute against Typesense 30.1 as well as earlier supported versions.