FEATURE: Can edit category/host relationships for embedding#1
FEATURE: Can edit category/host relationships for embedding#1akshayutture-augment wants to merge 1 commit into
Conversation
![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.
ℹ️ 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".
| 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 |
There was a problem hiding this comment.
The new embeddable host migration always executes SELECT … WHERE s.name = 'embed_category' and immediately indexes [0]['id']. On any site that never configured embed_category (which is the default state, so there is no row in site_settings), that query returns zero rows and [0] is nil, so the migration raises undefined method '[]' for nil and the whole deploy halts before any of the new functionality can run. The migration needs to check whether the query returned a row (e.g. via cmd_tuples or first) and default to SiteSetting.uncategorized_category_id when it didn’t, instead of indexing blindly.
Useful? React with 👍 / 👎.
| if records.present? | ||
| records.each do |h| | ||
| execute "INSERT INTO embeddable_hosts (host, category_id, created_at, updated_at) VALUES ('#{h}', #{category_id}, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)" |
There was a problem hiding this comment.
During migration each legacy host string from the embeddable_hosts setting is inserted verbatim into the new table. The previous implementation explicitly tolerated entries like http://example.com/path by stripping the scheme and path before comparison, but the new EmbeddableHost.host_allowed? compares only the bare hostname (URI(host).host). As a result, any existing host that still contains a protocol or path will now be stored with those characters and will never match an incoming embed request, silently disabling embedding for that host after upgrade. The migration should strip schemes/paths (or create the rows through EmbeddableHost so the before_validation callback runs) before inserting.
Useful? React with 👍 / 👎.
2 participants
Test 10
Replicated from ai-code-review-evaluation/discourse-cursor#10