Improve UI for home page - #10
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds localized form help, back navigation links, a keys preview column and totals, a conditional clear-cache action, admin search/filtering (concern, controller, model, partial), thread-local loader behavior, and updated specs and locales. ChangesTranslation Set Admin Interface Enhancements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the Decidim Term Customizer admin UI by extending the translation sets listing with additional informational columns and adding a “Clear cache” action to the translations list view.
Changes:
- Add i18n labels for new translation set table columns (“Keys”, “Total translations”).
- Add a “Clear cache” button to the translations index header actions.
- Extend translation sets index table to show a preview of translation keys and total translation count per set.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| config/locales/en.yml | Adds English labels for the new “Keys” and “Total translations” columns. |
| app/views/decidim/term_customizer/admin/translations/index.html.erb | Adds a “Clear cache” action button to the translations list view header. |
| app/views/decidim/term_customizer/admin/translation_sets/index.html.erb | Adds “Keys” preview and “Total translations” columns to the translation sets table. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/views/decidim/term_customizer/admin/translation_sets/index.html.erb`:
- Around line 25-26: The table header for the "total" column in the view
template is not closed; in the template
decidim/term_customizer/admin/translation_sets/index.html.erb the <th> that
renders t("models.translation_sets.fields.total", scope:
"decidim.term_customizer.admin") should be closed explicitly; update that <th>
element to include a corresponding closing </th> so the table markup is valid
(ensure the closing tag follows the t(...) call and before the next <th
class="actions">).
- Around line 39-43: Precompute translation previews and counts in the
controller instead of querying per row: build two hashes (e.g.,
`@preview_keys_by_set_id` mapping set.id -> array of up to 4 translation keys, and
`@translation_counts_by_set_id` mapping set.id -> integer count) by a single query
or batched queries (use group/pluck or a single JOIN + GROUP BY to collect keys
and counts), then update the view to use
`@preview_keys_by_set_id`[set.id].first(3).join(", ") with "..." if the array size
> 3 and `@translation_counts_by_set_id`[set.id] for the count; replace all uses of
set.translations.pluck and set.translations.count with these hash lookups (refer
to variables `@preview_keys_by_set_id` and `@translation_counts_by_set_id` and the
view's preview logic).
In `@app/views/decidim/term_customizer/admin/translations/index.html.erb`:
- Around line 25-27: Replace the generic destroy confirmation key used in the
Apply translations link with an action-specific locale key: change the data
confirm call in the link_to that currently uses t("actions.confirm_destroy",
scope: "decidim.admin") to a new key under the term_customizer admin scope (for
example t("actions.confirm_apply_translations", scope:
"decidim.term_customizer.admin")), and add the corresponding translation string
to the decidim.term_customizer.admin.actions locale entries so the confirmation
text correctly reflects the "Apply translations" action.
In `@config/locales/en.yml`:
- Line 29: The success toast for the renamed action is inconsistent: update the
locale entry for the clear_cache key in config/locales/en.yml so the success
message matches “Apply translations” (e.g., change the current “Cache cleared
successfully” text to “Translations applied successfully” or similar); ensure
the updated string is used wherever the clear_cache success toast is referenced
so UX copy remains consistent with the action rename.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0da5ce01-b5d9-4fb0-8da6-d2ecc57a056f
📒 Files selected for processing (6)
app/views/decidim/term_customizer/admin/translation_sets/_form.html.erbapp/views/decidim/term_customizer/admin/translation_sets/edit.html.erbapp/views/decidim/term_customizer/admin/translation_sets/index.html.erbapp/views/decidim/term_customizer/admin/translation_sets/new.html.erbapp/views/decidim/term_customizer/admin/translations/index.html.erbconfig/locales/en.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/controllers/concerns/decidim/term_customizer/admin/filterable.rb (1)
16-22: ⚡ Quick winConfirm
base_querymatchesOrganizationTranslationSets(and optionally refactor duplication).
Decidim::TermCustomizer::OrganizationTranslationSetsuses the same core filter asbase_query:TranslationSet.joins(:constraints).where(decidim_term_customizer_constraints: { decidim_organization_id: ... }). Both also de-duplicate (OrganizationTranslationSetsviaDISTINCT(id)in the select;base_queryvia.distinct), so the filtered result set should stay consistent.Consider extracting the shared “translation sets with constraints for an organization” relation/scope so
base_queryandOrganizationTranslationSetsdon’t diverge later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/concerns/decidim/term_customizer/admin/filterable.rb` around lines 16 - 22, The base_query implementation duplicates the same organization-filtered translation-set relation used in Decidim::TermCustomizer::OrganizationTranslationSets; update the code to ensure they match and avoid divergence by extracting the shared relation into a single scope or class method (e.g., on TranslationSet like with_constraints_for_organization or translation_sets_for_organization) that encapsulates TranslationSet.joins(:constraints).where(decidim_term_customizer_constraints: { decidim_organization_id: current_organization.id }).distinct, and then replace the inline query in both base_query and Decidim::TermCustomizer::OrganizationTranslationSets to call that new scope/method so both use the exact same logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/decidim/term_customizer/translation_set.rb`:
- Around line 18-20: Update the stale comment above the ransacker_i18n_multi
call in TranslationSet (the line with ransacker_i18n_multi :search_text,
[:name]) so it accurately reflects the implemented behavior: either change the
comment to say it creates i18n ransackers only for :name and that :search_text
aliases :name, or, if you intended to index both fields, modify the call to
ransacker_i18n_multi :search_text, [:name, :description] and keep the original
comment; ensure the comment and the ransacker_i18n_multi invocation stay
consistent.
---
Nitpick comments:
In `@app/controllers/concerns/decidim/term_customizer/admin/filterable.rb`:
- Around line 16-22: The base_query implementation duplicates the same
organization-filtered translation-set relation used in
Decidim::TermCustomizer::OrganizationTranslationSets; update the code to ensure
they match and avoid divergence by extracting the shared relation into a single
scope or class method (e.g., on TranslationSet like
with_constraints_for_organization or translation_sets_for_organization) that
encapsulates
TranslationSet.joins(:constraints).where(decidim_term_customizer_constraints: {
decidim_organization_id: current_organization.id }).distinct, and then replace
the inline query in both base_query and
Decidim::TermCustomizer::OrganizationTranslationSets to call that new
scope/method so both use the exact same logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31bb9b40-185b-4158-9217-282cec021933
📒 Files selected for processing (6)
app/controllers/concerns/decidim/term_customizer/admin/filterable.rbapp/controllers/decidim/term_customizer/admin/translation_sets_controller.rbapp/models/decidim/term_customizer/translation_set.rbapp/views/decidim/term_customizer/admin/translation_sets/_search.html.erbapp/views/decidim/term_customizer/admin/translation_sets/index.html.erbconfig/locales/en.yml
✅ Files skipped from review due to trivial changes (1)
- config/locales/en.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/decidim/term_customizer/admin/translation_sets/index.html.erb
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
spec/controllers/decidim/term_customizer/admin/add_translations_controller_spec.rb (1)
69-69: ⚡ Quick winAvoid hard-coding global translation count in this search spec.
Line 69 now pins the result size to
85, which is fragile and likely to keep changing as locales evolve. Assert on stable behavior (non-empty + expected key) instead.Proposed spec hardening
- expect(json.length).to eq(85) + expect(json).not_to be_empty + expect(json).to include( + "decidim.term_customizer.menu.term_customizer" => "Term customizer" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/controllers/decidim/term_customizer/admin/add_translations_controller_spec.rb` at line 69, The assertion expect(json.length).to eq(85) is brittle; replace it with stable assertions: ensure the returned json is non-empty (e.g., expect(json).not_to be_empty) and that it contains a known translation key used in this spec (e.g., expect(json).to include(<the_expected_translation_key>)). Update the test in add_translations_controller_spec.rb to remove the hard-coded 85 and assert presence of a specific expected key and non-emptiness using the existing json variable.spec/lib/decidim/term_customizer/translation_directory_spec.rb (1)
27-27: ⚡ Quick winThese assertions are coupled to mutable locale inventory.
Lines 27 and 37 hard-code
85, so future translation additions will break tests without functional regressions. Prefer invariant expectations: key presence and non-empty result.Proposed spec hardening
- expect(subject.translations_search("term_customizer").length).to eq(85) + results = subject.translations_search("term_customizer") + expect(results).not_to be_empty + expect(results).to include( + "decidim.term_customizer.menu.term_customizer" => "Term customizer" + ) ... - expect(subject.translations_by_key("term_customizer").length).to eq(85) + by_key = subject.translations_by_key("term_customizer") + expect(by_key).not_to be_empty + expect(by_key).to include( + "decidim.term_customizer.menu.term_customizer" => "Term customizer" + )Also applies to: 37-37
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/lib/decidim/term_customizer/translation_directory_spec.rb` at line 27, The spec is brittle because it asserts a fixed count (85) from subject.translations_search("term_customizer"); change the expectation to be invariant to locale additions by asserting the result is non-empty and contains specific expected keys instead of a hard-coded length. Locate the calls to subject.translations_search("term_customizer") in translation_directory_spec.rb and replace the eq(85) assertions with checks like expect(...).not_to be_empty and expect(keys).to include(<one_or_two_stable_translation_keys_for_term_customizer>) using the actual stable translation keys your app relies on.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/views/decidim/term_customizer/admin/translations/edit.html.erb`:
- Around line 16-19: The back-link calls translation_set_translations_path
without the required translation set argument; update the link_to invocation to
pass the existing set object (use translation_set_translations_path(set)) so URL
generation works, keeping the same classes and block content (icon and
t(".back")) and ensuring the local variable set used earlier in this template is
available to the helper.
---
Nitpick comments:
In
`@spec/controllers/decidim/term_customizer/admin/add_translations_controller_spec.rb`:
- Line 69: The assertion expect(json.length).to eq(85) is brittle; replace it
with stable assertions: ensure the returned json is non-empty (e.g.,
expect(json).not_to be_empty) and that it contains a known translation key used
in this spec (e.g., expect(json).to include(<the_expected_translation_key>)).
Update the test in add_translations_controller_spec.rb to remove the hard-coded
85 and assert presence of a specific expected key and non-emptiness using the
existing json variable.
In `@spec/lib/decidim/term_customizer/translation_directory_spec.rb`:
- Line 27: The spec is brittle because it asserts a fixed count (85) from
subject.translations_search("term_customizer"); change the expectation to be
invariant to locale additions by asserting the result is non-empty and contains
specific expected keys instead of a hard-coded length. Locate the calls to
subject.translations_search("term_customizer") in translation_directory_spec.rb
and replace the eq(85) assertions with checks like expect(...).not_to be_empty
and expect(keys).to
include(<one_or_two_stable_translation_keys_for_term_customizer>) using the
actual stable translation keys your app relies on.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b1145a4-df62-4524-8dd1-e6628f622345
📒 Files selected for processing (7)
app/models/decidim/term_customizer/translation_set.rbapp/views/decidim/term_customizer/admin/translation_sets/index.html.erbapp/views/decidim/term_customizer/admin/translations/edit.html.erbapp/views/decidim/term_customizer/admin/translations/index.html.erbconfig/locales/en.ymlspec/controllers/decidim/term_customizer/admin/add_translations_controller_spec.rbspec/lib/decidim/term_customizer/translation_directory_spec.rb
✅ Files skipped from review due to trivial changes (1)
- config/locales/en.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- app/views/decidim/term_customizer/admin/translations/index.html.erb
- app/models/decidim/term_customizer/translation_set.rb
- app/views/decidim/term_customizer/admin/translation_sets/index.html.erb
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/models/decidim/term_customizer/translation_set.rb (1)
26-28: Consider eager loading the translations association in the controller.Allowing Ransack to search through the
translationsassociation can introduce N+1 query performance issues if the association is not properly eager-loaded when the filtered collection is retrieved.Verify that the controller uses
.includes(:translations)or.eager_load(:translations)when building the filtered collection, especially if the index view displays translation counts or keys.Example controller pattern
def index `@translation_sets` = filtered_collection.includes(:translations) end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/decidim/term_customizer/translation_set.rb` around lines 26 - 28, The ransackable_associations method exposes the translations association for searching which can cause N+1 queries; update the controller action that builds the filtered collection (e.g., the index action where filtered_collection is used) to eager-load translations by chaining .includes(:translations) or .eager_load(:translations) when assigning `@translation_sets` (or the equivalent variable) so any view logic that accesses TranslationSet#translations, counts, or keys won’t trigger additional queries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/models/decidim/term_customizer/translation_set.rb`:
- Around line 26-28: The ransackable_associations method exposes the
translations association for searching which can cause N+1 queries; update the
controller action that builds the filtered collection (e.g., the index action
where filtered_collection is used) to eager-load translations by chaining
.includes(:translations) or .eager_load(:translations) when assigning
`@translation_sets` (or the equivalent variable) so any view logic that accesses
TranslationSet#translations, counts, or keys won’t trigger additional queries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e4661fc-7e23-45a0-a26d-09dbf9fd4051
📒 Files selected for processing (4)
app/controllers/concerns/decidim/term_customizer/admin/filterable.rbapp/models/decidim/term_customizer/translation.rbapp/models/decidim/term_customizer/translation_set.rbconfig/locales/en.yml
✅ Files skipped from review due to trivial changes (1)
- config/locales/en.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/concerns/decidim/term_customizer/admin/filterable.rb
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/decidim/term_customizer.rb`:
- Around line 40-46: The loader accessors currently use fiber-local storage via
Thread.current[:term_customizer_loader]; change them to use true thread-local
storage by replacing reads/writes with
Thread.current.thread_variable_get(:term_customizer_loader) and
Thread.current.thread_variable_set(:term_customizer_loader, loader) in the
loader and loader= methods respectively, and make sure any code that sets the
loader (e.g., request handling or initializer code that uses loader=) clears it
in an ensure/finally block so reused Puma threads never leak a previous
request's loader.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 075086e6-0e55-4362-bdc5-be5dbd54a653
📒 Files selected for processing (2)
lib/decidim/term_customizer.rblib/decidim/term_customizer/i18n_backend.rb
💤 Files with no reviewable changes (1)
- lib/decidim/term_customizer/i18n_backend.rb
Member
Author
|
@ElviaBth can we add searching entries by values? no only keys? |
Member
@microstudi, it is done. |
Merged
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
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.
🎩 What? Why?
Pending improvements:
📷 Screenshots
Add search by name and by translation keys or values
Summary by CodeRabbit
New Features
User Experience
Tests
Documentation