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
|
@cubic-dev-ai review this pull request |
@ShashankFC I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
18 issues found across 36 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="app/assets/javascripts/admin/components/embeddable-host.js.es6">
<violation number="1" location="app/assets/javascripts/admin/components/embeddable-host.js.es6:46">
P1: Missing error handling on `destroyRecord()`. The `save()` action properly uses `.catch(popupAjaxError)` but the `delete` action does not handle promise rejection. If the server returns an error, the user gets no feedback and a stale UI state.</violation>
</file>
<file name="app/controllers/admin/embeddable_hosts_controller.rb">
<violation number="1" location="app/controllers/admin/embeddable_hosts_controller.rb:10">
P1: No nil check after lookup — if the `EmbeddableHost` is not found, `save_host(host)` will raise a `NoMethodError` on nil. Use `find` (which raises `ActiveRecord::RecordNotFound` and returns 404) or add an explicit nil guard.</violation>
</file>
<file name="spec/models/topic_spec.rb">
<violation number="1" location="spec/models/topic_spec.rb:1407">
P3: Typo: `'with an emeddable host'` should be `'with an embeddable host'`.</violation>
</file>
<file name="spec/fabricators/embeddable_host_fabricator.rb">
<violation number="1" location="spec/fabricators/embeddable_host_fabricator.rb:1">
P1: File contents are swapped: `embeddable_host_fabricator.rb` contains Category fabricators instead of EmbeddableHost fabricators. The actual `Fabricator(:embeddable_host)` is defined in `category_fabricator.rb`. These file contents should be exchanged so each fabricator file matches its model name.</violation>
<violation number="2" location="spec/fabricators/embeddable_host_fabricator.rb:23">
P1: `after_build` calls `cat.update!` on an unpersisted record, which will raise an error. `update!` requires the record to be already saved to the database. Use `after_create` instead of `after_build` so the record is persisted before calling `update!`.</violation>
</file>
<file name="app/models/embeddable_host.rb">
<violation number="1" location="app/models/embeddable_host.rb:6">
P2: Potential `NoMethodError` if `host` is nil. The `before_validation` callback calls `self.host.sub!` without a nil guard. If a record is saved without setting `host`, this will crash before validation can reject it.</violation>
<violation number="2" location="app/models/embeddable_host.rb:17">
P1: Bug: case-insensitive lookup is broken because the query parameter is not downcased. `lower(host)` lowercases the column value but the bound parameter retains its original casing from `uri.host`, so mixed-case URLs will fail to match.</violation>
</file>
<file name="app/assets/javascripts/admin/controllers/admin-embedding.js.es6">
<violation number="1" location="app/assets/javascripts/admin/controllers/admin-embedding.js.es6:6">
P1: Bug: `update({})` is called with an empty object, so no data is actually sent to the server. The embeddable hosts data should be serialized and passed. Additionally, the returned promise is ignored — the user receives no success/failure feedback.</violation>
<violation number="2" location="app/assets/javascripts/admin/controllers/admin-embedding.js.es6:15">
P1: Bug: `deleteHost` only removes the host from the local array but never deletes it on the server. If the host was previously persisted, it will reappear after page reload. Following the codebase pattern (e.g., `admin-api.js.es6` `revokeKey`), a server-side destroy call should be made before or alongside the local removal.</violation>
</file>
<file name="app/assets/javascripts/discourse/adapters/rest.js.es6">
<violation number="1" location="app/assets/javascripts/discourse/adapters/rest.js.es6:22">
P2: `String.replace` with a string pattern only replaces the **first** occurrence. Use a regex with the global flag (`/_/g`) to ensure all underscores are converted to hyphens. This currently works by coincidence (each model has only one underscore), but will silently fail for any future model name with multiple underscores.</violation>
</file>
<file name="app/controllers/admin/embedding_controller.rb">
<violation number="1" location="app/controllers/admin/embedding_controller.rb:3">
P2: `:ensure_logged_in` and `:ensure_staff` are already declared in the parent `Admin::AdminController` and will be inherited. Only `:fetch_embedding` needs to be added here.</violation>
<violation number="2" location="app/controllers/admin/embedding_controller.rb:9">
P1: Bug: `update` action does not process any parameters or persist changes — it just re-renders the current state, making it a no-op identical to `show`. The action should read permitted params, update the relevant records (e.g., `EmbeddableHost` attributes), and then render the result.</violation>
</file>
<file name="db/migrate/20150818190757_create_embeddable_hosts.rb">
<violation number="1" location="db/migrate/20150818190757_create_embeddable_hosts.rb:3">
P2: `force: true` will silently drop and recreate the `embeddable_hosts` table if it already exists, causing data loss. This is inappropriate in a migration; remove `force: true` so the migration fails cleanly if the table already exists.</violation>
<violation number="2" location="db/migrate/20150818190757_create_embeddable_hosts.rb:9">
P1: Potential `NoMethodError`: if the `embed_category` setting doesn't exist or doesn't match any category name, this query returns no rows. Calling `[0]['id']` on an empty result raises `NoMethodError` on `nil`, and the fallback on line 14 is never reached. The result should be checked for emptiness first.</violation>
<violation number="3" location="db/migrate/20150818190757_create_embeddable_hosts.rb:25">
P1: SQL injection: `h` is interpolated directly into the SQL string without escaping. If the stored host value contains a single quote, this will either break the migration or allow SQL injection. Use `connection.quote` to safely escape the value.</violation>
</file>
<file name="spec/controllers/embed_controller_spec.rb">
<violation number="1" location="spec/controllers/embed_controller_spec.rb:70">
P2: Passing full URLs to `host:` relies on implicit `before_validation` stripping. For `'https://example.com/1234'`, the path `/1234` is silently dropped, so the host stored is just `example.com`. This means the downstream test "works with a host with a path" doesn't actually verify path-based host matching—it's just matching on the bare domain. Consider passing clean hostnames directly (e.g., `host: 'example.com'`) to make the test setup explicit, or if path-based matching is intended, the model and test need to be updated to support it.</violation>
</file>
<file name="app/assets/javascripts/discourse/models/store.js.es6">
<violation number="1" location="app/assets/javascripts/discourse/models/store.js.es6:197">
P1: Missing null/undefined guard on `obj[k]` before calling `.map()`. If a property ending in `_ids` has a null or non-array value, this will throw a TypeError at runtime. Add a guard to check that `obj[k]` is an array before mapping.</violation>
<violation number="2" location="app/assets/javascripts/discourse/models/store.js.es6:200">
P1: `delete obj[k]` is unconditional in the plural branch, unlike the singular branch which only deletes when hydration succeeds. This loses the original IDs even when `_lookupSubType` returns `undefined` for entries. Also, `hydrated || []` is dead code since `.map()` always returns an array. Consider filtering out undefined results and only deleting `obj[k]` if hydration produced valid results.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| delete() { | ||
| bootbox.confirm(I18n.t('admin.embedding.confirm_delete'), (result) => { | ||
| if (result) { | ||
| this.get('host').destroyRecord().then(() => { |
There was a problem hiding this comment.
P1: Missing error handling on destroyRecord(). The save() action properly uses .catch(popupAjaxError) but the delete action does not handle promise rejection. If the server returns an error, the user gets no feedback and a stale UI state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/assets/javascripts/admin/components/embeddable-host.js.es6, line 46:
<comment>Missing error handling on `destroyRecord()`. The `save()` action properly uses `.catch(popupAjaxError)` but the `delete` action does not handle promise rejection. If the server returns an error, the user gets no feedback and a stale UI state.</comment>
<file context>
@@ -0,0 +1,63 @@
+ delete() {
+ bootbox.confirm(I18n.t('admin.embedding.confirm_delete'), (result) => {
+ if (result) {
+ this.get('host').destroyRecord().then(() => {
+ this.sendAction('deleteHost', this.get('host'));
+ });
</file context>
| end | ||
|
|
||
| def update | ||
| host = EmbeddableHost.where(id: params[:id]).first |
There was a problem hiding this comment.
P1: No nil check after lookup — if the EmbeddableHost is not found, save_host(host) will raise a NoMethodError on nil. Use find (which raises ActiveRecord::RecordNotFound and returns 404) or add an explicit nil guard.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/controllers/admin/embeddable_hosts_controller.rb, line 10:
<comment>No nil check after lookup — if the `EmbeddableHost` is not found, `save_host(host)` will raise a `NoMethodError` on nil. Use `find` (which raises `ActiveRecord::RecordNotFound` and returns 404) or add an explicit nil guard.</comment>
<file context>
@@ -0,0 +1,34 @@
+ end
+
+ def update
+ host = EmbeddableHost.where(id: params[:id]).first
+ save_host(host)
+ end
</file context>
| name 'Private Category' | ||
| slug 'private' | ||
| user | ||
| after_build do |cat, transients| |
There was a problem hiding this comment.
P1: after_build calls cat.update! on an unpersisted record, which will raise an error. update! requires the record to be already saved to the database. Use after_create instead of after_build so the record is persisted before calling update!.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/fabricators/embeddable_host_fabricator.rb, line 23:
<comment>`after_build` calls `cat.update!` on an unpersisted record, which will raise an error. `update!` requires the record to be already saved to the database. Use `after_create` instead of `after_build` so the record is persisted before calling `update!`.</comment>
<file context>
@@ -0,0 +1,27 @@
+ name 'Private Category'
+ slug 'private'
+ user
+ after_build do |cat, transients|
+ cat.update!(read_restricted: true)
+ cat.category_groups.build(group_id: transients[:group].id, permission_type: CategoryGroup.permission_types[:full])
</file context>
| @@ -0,0 +1,27 @@ | |||
| Fabricator(:category) do | |||
There was a problem hiding this comment.
P1: File contents are swapped: embeddable_host_fabricator.rb contains Category fabricators instead of EmbeddableHost fabricators. The actual Fabricator(:embeddable_host) is defined in category_fabricator.rb. These file contents should be exchanged so each fabricator file matches its model name.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/fabricators/embeddable_host_fabricator.rb, line 1:
<comment>File contents are swapped: `embeddable_host_fabricator.rb` contains Category fabricators instead of EmbeddableHost fabricators. The actual `Fabricator(:embeddable_host)` is defined in `category_fabricator.rb`. These file contents should be exchanged so each fabricator file matches its model name.</comment>
<file context>
@@ -0,0 +1,27 @@
+Fabricator(:category) do
+ name { sequence(:name) { |n| "Amazing Category #{n}" } }
+ user
</file context>
| host = uri.host | ||
| return false unless host.present? | ||
|
|
||
| where("lower(host) = ?", host).first |
There was a problem hiding this comment.
P1: Bug: case-insensitive lookup is broken because the query parameter is not downcased. lower(host) lowercases the column value but the bound parameter retains its original casing from uri.host, so mixed-case URLs will fail to match.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/models/embeddable_host.rb, line 17:
<comment>Bug: case-insensitive lookup is broken because the query parameter is not downcased. `lower(host)` lowercases the column value but the bound parameter retains its original casing from `uri.host`, so mixed-case URLs will fail to match.</comment>
<file context>
@@ -0,0 +1,24 @@
+ host = uri.host
+ return false unless host.present?
+
+ where("lower(host) = ?", host).first
+ end
+
</file context>
|
|
||
| basePath(store, type) { | ||
| if (ADMIN_MODELS.indexOf(type) !== -1) { return "/admin/"; } | ||
| if (ADMIN_MODELS.indexOf(type.replace('_', '-')) !== -1) { return "/admin/"; } |
There was a problem hiding this comment.
P2: String.replace with a string pattern only replaces the first occurrence. Use a regex with the global flag (/_/g) to ensure all underscores are converted to hyphens. This currently works by coincidence (each model has only one underscore), but will silently fail for any future model name with multiple underscores.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/assets/javascripts/discourse/adapters/rest.js.es6, line 22:
<comment>`String.replace` with a string pattern only replaces the **first** occurrence. Use a regex with the global flag (`/_/g`) to ensure all underscores are converted to hyphens. This currently works by coincidence (each model has only one underscore), but will silently fail for any future model name with multiple underscores.</comment>
<file context>
@@ -19,7 +19,7 @@ function rethrow(error) {
basePath(store, type) {
- if (ADMIN_MODELS.indexOf(type) !== -1) { return "/admin/"; }
+ if (ADMIN_MODELS.indexOf(type.replace('_', '-')) !== -1) { return "/admin/"; }
return "/";
},
</file context>
Suggested change
| if (ADMIN_MODELS.indexOf(type.replace('_', '-')) !== -1) { return "/admin/"; } | |
| if (ADMIN_MODELS.indexOf(type.replace(/_/g, '-')) !== -1) { return "/admin/"; } |
| @@ -0,0 +1,21 @@ | |||
| class Admin::EmbeddingController < Admin::AdminController | |||
|
|
|||
| before_filter :ensure_logged_in, :ensure_staff, :fetch_embedding | |||
There was a problem hiding this comment.
P2: :ensure_logged_in and :ensure_staff are already declared in the parent Admin::AdminController and will be inherited. Only :fetch_embedding needs to be added here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/controllers/admin/embedding_controller.rb, line 3:
<comment>`:ensure_logged_in` and `:ensure_staff` are already declared in the parent `Admin::AdminController` and will be inherited. Only `:fetch_embedding` needs to be added here.</comment>
<file context>
@@ -0,0 +1,21 @@
+class Admin::EmbeddingController < Admin::AdminController
+
+ before_filter :ensure_logged_in, :ensure_staff, :fetch_embedding
+
+ def show
</file context>
| @@ -0,0 +1,33 @@ | |||
| class CreateEmbeddableHosts < ActiveRecord::Migration | |||
| def change | |||
| create_table :embeddable_hosts, force: true do |t| | |||
There was a problem hiding this comment.
P2: force: true will silently drop and recreate the embeddable_hosts table if it already exists, causing data loss. This is inappropriate in a migration; remove force: true so the migration fails cleanly if the table already exists.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At db/migrate/20150818190757_create_embeddable_hosts.rb, line 3:
<comment>`force: true` will silently drop and recreate the `embeddable_hosts` table if it already exists, causing data loss. This is inappropriate in a migration; remove `force: true` so the migration fails cleanly if the table already exists.</comment>
<file context>
@@ -0,0 +1,33 @@
+class CreateEmbeddableHosts < ActiveRecord::Migration
+ def change
+ create_table :embeddable_hosts, force: true do |t|
+ t.string :host, null: false
+ t.integer :category_id, null: false
</file context>
| SiteSetting.embeddable_hosts = "#{host}\nhttp://discourse.org\nhttps://example.com/1234" | ||
| Fabricate(:embeddable_host) | ||
| Fabricate(:embeddable_host, host: 'http://discourse.org') | ||
| Fabricate(:embeddable_host, host: 'https://example.com/1234') |
There was a problem hiding this comment.
P2: Passing full URLs to host: relies on implicit before_validation stripping. For 'https://example.com/1234', the path /1234 is silently dropped, so the host stored is just example.com. This means the downstream test "works with a host with a path" doesn't actually verify path-based host matching—it's just matching on the bare domain. Consider passing clean hostnames directly (e.g., host: 'example.com') to make the test setup explicit, or if path-based matching is intended, the model and test need to be updated to support it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/controllers/embed_controller_spec.rb, line 70:
<comment>Passing full URLs to `host:` relies on implicit `before_validation` stripping. For `'https://example.com/1234'`, the path `/1234` is silently dropped, so the host stored is just `example.com`. This means the downstream test "works with a host with a path" doesn't actually verify path-based host matching—it's just matching on the bare domain. Consider passing clean hostnames directly (e.g., `host: 'example.com'`) to make the test setup explicit, or if path-based matching is intended, the model and test need to be updated to support it.</comment>
<file context>
@@ -68,7 +65,9 @@
- SiteSetting.embeddable_hosts = "#{host}\nhttp://discourse.org\nhttps://example.com/1234"
+ Fabricate(:embeddable_host)
+ Fabricate(:embeddable_host, host: 'http://discourse.org')
+ Fabricate(:embeddable_host, host: 'https://example.com/1234')
end
</file context>
| SiteSetting.embed_truncate = false | ||
| expect(topic.expandable_first_post?).to eq(false) | ||
| end | ||
| describe 'with an emeddable host' do |
There was a problem hiding this comment.
P3: Typo: 'with an emeddable host' should be 'with an embeddable host'.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/models/topic_spec.rb, line 1407:
<comment>Typo: `'with an emeddable host'` should be `'with an embeddable host'`.</comment>
<file context>
@@ -1397,32 +1397,34 @@ def build_topic_with_title(title)
- SiteSetting.embed_truncate = false
- expect(topic.expandable_first_post?).to eq(false)
- end
+ describe 'with an emeddable host' do
+ before do
+ Fabricate(:embeddable_host)
</file context>
Suggested change
| describe 'with an emeddable host' do | |
| describe 'with an embeddable host' do |
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.
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