FEATURE: Can edit category/host relationships for embedding#1
Open
ShashankFC wants to merge 1 commit into
Open
FEATURE: Can edit category/host relationships for embedding#1ShashankFC wants to merge 1 commit into
ShashankFC wants to merge 1 commit into
Conversation
Author
|
@CodeAnt-AI: review |
|
CodeAnt AI is running the review. |
codeant-ai
Bot
added
the
size:L
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
|
|
||
| def update | ||
| host = EmbeddableHost.where(id: params[:id]).first | ||
| save_host(host) |
There was a problem hiding this comment.
Suggestion: When updating a host, if the requested record ID does not exist, EmbeddableHost.where(...).first will return nil and save_host(host) will attempt to call methods on nil, causing a 500 error instead of a proper not-found response. [null pointer]
Severity Level: Major ⚠️
- ❌ Admin update host endpoint 500s for missing records.
- ⚠️ Admin embedding UI breaks when editing deleted host.
Suggested change
| save_host(host) | |
| raise Discourse::NotFound unless host |
Steps of Reproduction ✅
1. Log in as a staff user and open the admin embedding UI at `GET
/admin/customize/embedding` (routed via `config/routes.rb:138-139` to
`Admin::EmbeddingController#show`).
2. From that page, obtain an existing embeddable host ID (records are loaded from
`EmbeddableHost.all.order(:host)` in
`app/controllers/admin/embedding_controller.rb:15-19`) and then delete that record in
another session or console (so the original ID becomes stale).
3. In the first browser session, attempt to update the now-deleted host by issuing `PUT
/admin/embeddable_hosts/:id.json` with the stale ID (route defined at
`config/routes.rb:42-43,153` mapping to `Admin::EmbeddableHostsController#update`).
4. The controller action at `app/controllers/admin/embeddable_hosts_controller.rb:9-12`
executes `host = EmbeddableHost.where(id: params[:id]).first`, which returns `nil` for the
stale ID, then calls `save_host(host)`, and inside `save_host` at
`app/controllers/admin/embeddable_hosts_controller.rb:22-25` the line `host.host = ...`
raises `NoMethodError` on `nil`, producing an unhandled 500 error instead of the 404
behavior implemented for `Discourse::NotFound` in
`app/controllers/application_controller.rb:108-110`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** app/controllers/admin/embeddable_hosts_controller.rb
**Line:** 11:11
**Comment:**
*Null Pointer: When updating a host, if the requested record ID does not exist, `EmbeddableHost.where(...).first` will return `nil` and `save_host(host)` will attempt to call methods on `nil`, causing a 500 error instead of a proper not-found response.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.|
|
||
| def destroy | ||
| host = EmbeddableHost.where(id: params[:id]).first | ||
| host.destroy |
There was a problem hiding this comment.
Suggestion: When destroying a host, if the given ID does not exist, EmbeddableHost.where(...).first returns nil and host.destroy will raise an exception, producing a 500 error instead of a clean 404-style response. [null pointer]
Severity Level: Major ⚠️
- ❌ Admin delete host endpoint 500s when ID is stale.
- ⚠️ Admin embedding UI misbehaves deleting already-removed host.
Suggested change
| host.destroy | |
| raise Discourse::NotFound unless host |
Steps of Reproduction ✅
1. Log in as a staff user and open the admin embedding UI at `GET
/admin/customize/embedding` (routed via `config/routes.rb:138-139` to
`Admin::EmbeddingController#show`, which lists `EmbeddableHost` records).
2. In one session or Rails console, delete a specific `EmbeddableHost` record directly so
that its ID becomes stale for any open admin page.
3. In another browser tab where the old list is still loaded, attempt to delete that stale
host by issuing `DELETE /admin/embeddable_hosts/:id.json` with the stale ID (resource
route defined at `config/routes.rb:42-43,153` to
`Admin::EmbeddableHostsController#destroy`).
4. The destroy action at `app/controllers/admin/embeddable_hosts_controller.rb:14-18` runs
`host = EmbeddableHost.where(id: params[:id]).first`, which returns `nil` for the
non-existent ID, then immediately calls `host.destroy`, raising `NoMethodError` on `nil`
and returning a 500 error instead of the `Discourse::NotFound`-driven 404 handled in
`app/controllers/application_controller.rb:108-110`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** app/controllers/admin/embeddable_hosts_controller.rb
**Line:** 16:16
**Comment:**
*Null Pointer: When destroying a host, if the given ID does not exist, `EmbeddableHost.where(...).first` returns `nil` and `host.destroy` will raise an exception, producing a 500 error instead of a clean 404-style response.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+12
to
+16
| return false unless uri.present? | ||
|
|
||
| host = uri.host | ||
| return false unless host.present? | ||
|
|
There was a problem hiding this comment.
Suggestion: record_for_host currently uses only uri.host when looking up the embeddable host, ignoring any non-standard port; this means a host saved as example.com:3000 will never match a referer like http://example.com:3000/..., causing host_allowed? to return false and valid embeds on custom ports to be rejected. Include the port in the lookup key when present so records with ports can be found correctly. [logic error]
Severity Level: Critical 🚨
- ❌ Embeds from hosts on nonstandard ports always rejected.
- ⚠️ TopicRetriever invalid_host? misclassifies allowed port-specific hosts.
- ⚠️ New embedded topics miscategorized when using port-specific hosts.
Suggested change
| return false unless uri.present? | |
| host = uri.host | |
| return false unless host.present? | |
| return false unless uri && uri.host.present? | |
| base_host = uri.host.downcase | |
| host_with_port = if uri.port && uri.port != 80 && uri.port != 443 | |
| "#{base_host}:#{uri.port}" | |
| else | |
| base_host | |
| end | |
| where("lower(host) = ?", host_with_port).first |
Steps of Reproduction ✅
1. Using `Admin::EmbeddableHostsController#create` in
`app/controllers/admin/embeddable_hosts_controller.rb:5-7`, create an `EmbeddableHost`
with `host` set to `"example.com:3000"`; `save_host` at lines 22-27 assigns this string
directly and the `before_validation` callback in `app/models/embeddable_host.rb:5-8`
leaves the `:3000` port intact.
2. Host a page at `http://example.com:3000/article` that uses Discourse embeds so that the
browser loads `EmbedController#comments` in `app/controllers/embed_controller.rb:7-35`
with `request.referer` equal to `"http://example.com:3000/article"`.
3. Before the action runs, the `ensure_embeddable` filter in
`app/controllers/embed_controller.rb:56-62` executes and calls
`EmbeddableHost.host_allowed?(request.referer)` (line 61), which delegates to
`EmbeddableHost.record_for_host` in `app/models/embeddable_host.rb:10-21`.
4. `record_for_host` parses the referer, sets `host = uri.host` (line 14, e.g.
`"example.com"` without `:3000`), and queries `where("lower(host) = ?", host).first` (line
17), which does not match the stored `"example.com:3000"` record; `host_allowed?` returns
`false` and `ensure_embeddable` raises `Discourse::InvalidAccess.new('invalid referer
host')`, so all embeds from `example.com:3000` are rejected.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** app/models/embeddable_host.rb
**Line:** 12:16
**Comment:**
*Logic Error: `record_for_host` currently uses only `uri.host` when looking up the embeddable host, ignoring any non-standard port; this means a host saved as `example.com:3000` will never match a referer like `http://example.com:3000/...`, causing `host_allowed?` to return false and valid embeds on custom ports to be rejected. Include the port in the lookup key when present so records with ports can be found correctly.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+9
to
+14
| category_id = execute("SELECT c.id FROM categories AS c | ||
| INNER JOIN site_settings AS s ON s.value = c.name | ||
| WHERE s.name = 'embed_category'")[0]['id'].to_i | ||
|
|
||
|
|
||
| if category_id == 0 |
There was a problem hiding this comment.
Suggestion: The migration assumes that the SELECT for embed_category always returns at least one row and immediately indexes [0]['id'], which will raise a NoMethodError if the embed_category site setting is missing or does not match any category; this will cause the migration to fail on such installs instead of cleanly falling back to the uncategorized category as intended. [null pointer]
Severity Level: Critical 🚨
- X Database migration 20150818190757 fails on fresh installs.
- ! Embeddable hosts table never created; admin embedding UI breaks.
- X Entire db:migrate task aborts, blocking application deployment.
Suggested change
| category_id = execute("SELECT c.id FROM categories AS c | |
| INNER JOIN site_settings AS s ON s.value = c.name | |
| WHERE s.name = 'embed_category'")[0]['id'].to_i | |
| if category_id == 0 | |
| category_id_result = execute("SELECT c.id FROM categories AS c | |
| INNER JOIN site_settings AS s ON s.value = c.name | |
| WHERE s.name = 'embed_category'") | |
| if category_id_result && category_id_result.cmd_tuples > 0 | |
| category_id = category_id_result[0]['id'].to_i | |
| else |
Steps of Reproduction ✅
1. Start from a Discourse database where no `site_settings` row exists with `name =
'embed_category'` (this key does not appear in `config/site_settings.yml`, confirmed by
Grep, so fresh installs will not have it).
2. Run `rake db:migrate` so Rails executes the migration `CreateEmbeddableHosts` in
`db/migrate/20150818190757_create_embeddable_hosts.rb:1-33`.
3. During the `change` method, the line at
`db/migrate/20150818190757_create_embeddable_hosts.rb:9-11` executes: `category_id =
execute("SELECT c.id FROM categories AS c INNER JOIN site_settings AS s ON s.value =
c.name WHERE s.name = 'embed_category'")[0]['id'].to_i`.
4. Because the query returns zero rows, `execute` yields a `PG::Result` with no tuples;
`result[0]` is `nil`, so `result[0]['id']` raises `NoMethodError: undefined method '[]'
for nil:NilClass`, aborting the migration before the fallback to
`uncategorized_category_id` at lines 14-16 can run.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** db/migrate/20150818190757_create_embeddable_hosts.rb
**Line:** 9:14
**Comment:**
*Null Pointer: The migration assumes that the `SELECT` for `embed_category` always returns at least one row and immediately indexes `[0]['id']`, which will raise a `NoMethodError` if the `embed_category` site setting is missing or does not match any category; this will cause the migration to fail on such installs instead of cleanly falling back to the uncategorized category as intended.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+116
to
+117
| assert.equal(fruitCols[0].get('id'), 1); | ||
| assert.equal(fruitCols[1].get('id'), 2); |
There was a problem hiding this comment.
Suggestion: fruitCols is an Ember relationship array (e.g., a ManyArray/ArrayProxy), so indexing it with [0]/[1] will return undefined rather than the related records, causing fruitCols[0].get('id') and fruitCols[1].get('id') to throw at runtime; use .objectAt(index) instead to safely access items. [type error]
Severity Level: Major ⚠️
- ❌ QUnit test `find embedded` in store-test fails.
- ⚠️ JS test suite / CI cannot complete successfully.
- ⚠️ Embedded `fruit.colors` relationship remains unverified by tests.
Suggested change
| assert.equal(fruitCols[0].get('id'), 1); | |
| assert.equal(fruitCols[1].get('id'), 2); | |
| assert.equal(fruitCols.objectAt(0).get('id'), 1); | |
| assert.equal(fruitCols.objectAt(1).get('id'), 2); |
Steps of Reproduction ✅
1. Run the JavaScript test suite so that the `module('store:main');` tests in
`test/javascripts/models/store-test.js.es6:1` are executed.
2. The test `test('find embedded', function(assert) { ... })` at
`test/javascripts/models/store-test.js.es6:109-121` creates a store via `createStore()`
(imported from `test/javascripts/helpers/create-store.js.es6:1-23`) and calls
`store.find('fruit', 2)` at line 111.
3. The Pretender helper in `test/javascripts/helpers/create-pretender.js.es6` defines
`fruits` and `colors` arrays and REST endpoints `/fruits/:id` and `/fruits` (see lines
around `42` and `226-232`), so the store returns a `fruit` model whose `colors`
relationship at `store-test.js.es6:114` is a relationship collection
(ManyArray/ArrayProxy) with `length === 2`.
4. At `test/javascripts/models/store-test.js.es6:114-117`, the test executes `const
fruitCols = f.get('colors');` followed by `assert.equal(fruitCols[0].get('id'), 1);` and
`assert.equal(fruitCols[1].get('id'), 2);`: because Ember relationship arrays are
`ArrayProxy`-like and not plain JS arrays, `fruitCols[0]` and `fruitCols[1]` are
`undefined` even though `fruitCols.length === 2`, so `fruitCols[0].get('id')` throws a
TypeError (`Cannot read property 'get' of undefined`), causing the `find embedded` test to
fail.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** test/javascripts/models/store-test.js.es6
**Line:** 116:117
**Comment:**
*Type Error: `fruitCols` is an Ember relationship array (e.g., a ManyArray/ArrayProxy), so indexing it with `[0]`/`[1]` will return `undefined` rather than the related records, causing `fruitCols[0].get('id')` and `fruitCols[1].get('id')` to throw at runtime; use `.objectAt(index)` instead to safely access items.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+131
to
+132
| assert.equal(fruitCols[0].get('id'), 1); | ||
| assert.equal(fruitCols[1].get('id'), 2); |
There was a problem hiding this comment.
Suggestion: In the second embedded test, fruitCols is again an Ember relationship array, so using fruitCols[0]/fruitCols[1] will yield undefined and fruitCols[0].get('id') will fail; use .objectAt() to access items safely. [type error]
Severity Level: Major ⚠️
- ❌ QUnit test `findAll embedded` in store-test fails.
- ⚠️ Prevents validating shared farmer/color embedding behavior.
- ⚠️ JS test suite / CI stability degraded by failing tests.
Suggested change
| assert.equal(fruitCols[0].get('id'), 1); | |
| assert.equal(fruitCols[1].get('id'), 2); | |
| assert.equal(fruitCols.objectAt(0).get('id'), 1); | |
| assert.equal(fruitCols.objectAt(1).get('id'), 2); |
Steps of Reproduction ✅
1. Run the JavaScript test suite so that the `module('store:main');` tests in
`test/javascripts/models/store-test.js.es6:1` are executed.
2. The test `test('findAll embedded', function(assert) { ... })` at
`test/javascripts/models/store-test.js.es6:123-135` creates a store and calls
`store.findAll('fruit')` at line 125, which hits the Pretender `/fruits` endpoint defined
in `test/javascripts/helpers/create-pretender.js.es6:231-232` returning `fruits`,
`farmers`, and `colors`.
3. The store returns a collection `fruits`; the test then executes `const fruitCols =
fruits.objectAt(0).get('colors');` at line 129 so that `fruitCols` is the `colors`
relationship collection (ManyArray/ArrayProxy) for the first fruit, with `fruitCols.length
=== 2` (matching the `color_ids: [1,2]` setup in `create-pretender.js.es6:42-49`).
4. Immediately after, lines 130-132 in `test/javascripts/models/store-test.js.es6` call
`fruitCols[0].get('id')` and `fruitCols[1].get('id')`; since Ember relationship arrays are
proxies rather than native arrays, `fruitCols[0]`/`fruitCols[1]` are `undefined` despite
the correct `length`, so these `.get('id')` calls throw a TypeError and the `findAll
embedded` test fails.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** test/javascripts/models/store-test.js.es6
**Line:** 131:132
**Comment:**
*Type Error: In the second embedded test, `fruitCols` is again an Ember relationship array, so using `fruitCols[0]`/`fruitCols[1]` will yield `undefined` and `fruitCols[0].get('id')` will fail; use `.objectAt()` to access items safely.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished running the review. |
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
2 participants
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.
User description
Test 10
Summary by CodeRabbit
Release Notes
New Features
Refactoring
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/discourse-coderabbit#10
CodeAnt-AI Description
Manage allowed embedding hosts from the admin UI and assign per-host categories
What Changed
Impact
✅ Admins can manage allowed embed hosts in UI✅ Embedded posts go to host-specific categories✅ Fewer configuration errors from site-setting to DB migration💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.