Skip to content

fix(intelligent-assistant): add singular/plural handling for notebook card resource count (RHDHBUGS-3582) - #4208

Open
rohitratannagar wants to merge 3 commits into
redhat-developer:mainfrom
rohitratannagar:roxie/rhdhbugs-3582
Open

fix(intelligent-assistant): add singular/plural handling for notebook card resource count (RHDHBUGS-3582)#4208
rohitratannagar wants to merge 3 commits into
redhat-developer:mainfrom
rohitratannagar:roxie/rhdhbugs-3582

Conversation

@rohitratannagar

Copy link
Copy Markdown
Contributor

Summary

  • Fix pluralization on the NotebookCard component so "1 Resource" vs "N Resources" renders correctly based on document count
  • Replace static notebooks.documents translation key with notebooks.documents_one / notebooks.documents_other plural keys across all supported locales (en, de, es, fr, it, ja)

Fixed

  • RHDHBUGS-3582 — Notebook document upload count pluralization error

Fixes: https://redhat.atlassian.net/browse/RHDHBUGS-3582

Checklist

  • Code compiles without errors (yarn tsc:full)
  • Tests pass (yarn test --watchAll=false)
  • Translation keys added for all supported locales
  • Signed-off-by included in commit

Made with Cursor

… card resource count

The NotebookCard on the notebooks listing page always displayed
"Resources" (plural) regardless of count. Replace the static
notebooks.documents key with notebooks.documents_one/_other plural
keys so "1 Resource" vs "2 Resources" renders correctly.

Co-authored-by: Cursor <cursoragent@cursor.com>
@rhdh-gh-app

rhdh-gh-app Bot commented Aug 7, 2026

Copy link
Copy Markdown

Changed Packages

Package Name Package Path Changeset Bump Current Version
@red-hat-developer-hub/backstage-plugin-intelligent-assistant workspaces/intelligent-assistant/plugins/intelligent-assistant patch v3.2.0

@rhdh-qodo-merge

Copy link
Copy Markdown

PR Summary by Qodo

Fix pluralized notebook resource counts via i18n plural keys

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Render notebook/document resource counts with correct singular vs plural label.
• Introduce _one/_other translation keys for resource counts across all locales.
• Add unit test coverage for 0/1/N resource count rendering and update e2e formatting.
Diagram

graph TD
  NC["NotebookCard.tsx"] --> I18N[("i18n t()")]
  DS["DocumentSidebar.tsx"] --> I18N[("i18n t()")]
  I18N[("i18n t()")]
  --> TR["Translations (ref + locales)"] --> API["report-alpha.api.md"]
  UT[["Unit tests"]] --> NC["NotebookCard.tsx"]
  UT[["Unit tests"]] --> DS["DocumentSidebar.tsx"]
  E2E[["E2E page object"]] --> TR["Translations (ref + locales)"]
  subgraph Legend
    direction LR
    _c["UI Component"] ~~~ _i[("Translations")]
    _t[["Tests"]] ~~~ _d["API contract"]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Keep base keys in the translation key union
  • ➕ Avoids (t as Function) casts while still leveraging i18n plural resolution
  • ➕ Preserves type-safety and autocompletion for the key actually used in code
  • ➖ Translation contract would list keys that don't exist as literals in catalogs (base key is implicit)
2. Use ICU-style message formatting for pluralization
  • ➕ Single key per message with explicit plural logic
  • ➕ More expressive for languages with complex plural rules
  • ➖ Requires ICU message support/configuration and consistent authoring patterns
  • ➖ Bigger change footprint than this targeted fix
3. E2E: call the same i18n plural resolver instead of manual key selection
  • ➕ Matches production behavior exactly (incl. locale-specific plural rules)
  • ➕ Avoids manual replace(&#x27;{{count}}&#x27;, ...) formatting
  • ➖ May require exposing i18n instance/helpers to tests or page objects
  • ➖ Slightly more setup in the e2e framework

Recommendation: The PR’s approach (i18n count-driven plural resolution with _one/_other keys) is the right minimal fix for RHDHBUGS-3582 and is consistent with i18next-style pluralization. Consider a small follow-up to restore type-safety by including the base keys (notebooks.documents, notebook.view.documents.count) in the translation key type definitions so the UI can drop (t as Function) casts, and optionally refactor the e2e formatting helper to use the same plural resolution mechanism as the app.

Files changed (12) +69 / -25

Bug fix (3) +12 / -6
NotebookSurfacePage.tsUpdate e2e expected label formatting for plural resource counts +7/-3

Update e2e expected label formatting for plural resource counts

• Adjusts the notebook card summary formatter to choose '_one' vs '_other' keys and substitute '{{count}}' for assertions. Updates inline docs to reflect pluralized translation behavior.

workspaces/intelligent-assistant/e2e-tests/pages/NotebookSurfacePage.ts

DocumentSidebar.tsxUse i18n pluralization with 'count' for sidebar document total +2/-2

Use i18n pluralization with 'count' for sidebar document total

• Switches the resource count rendering to call 't(..., { count })' so plural forms resolve correctly. Uses a function cast to accommodate typing around plural keys.

workspaces/intelligent-assistant/plugins/intelligent-assistant/src/components/notebooks/DocumentSidebar.tsx

NotebookCard.tsxPluralize NotebookCard resource count via i18n 'count' +3/-1

Pluralize NotebookCard resource count via i18n 'count'

• Replaces the manual '{count} {t('...')}' rendering with a pluralized translation call using 'count', enabling correct singular vs plural output.

workspaces/intelligent-assistant/plugins/intelligent-assistant/src/components/notebooks/NotebookCard.tsx

Tests (2) +29 / -5
DocumentSidebar.test.tsxAdd singular/plural assertions for DocumentSidebar resource count +17/-0

Add singular/plural assertions for DocumentSidebar resource count

• Introduces tests that verify the sidebar renders '1 Resource' for a single document and 'N Resources' for multiple documents.

workspaces/intelligent-assistant/plugins/intelligent-assistant/src/components/tests/DocumentSidebar.test.tsx

NotebookCard.test.tsxStrengthen NotebookCard count tests for 0/1/N resource cases +12/-5

Strengthen NotebookCard count tests for 0/1/N resource cases

• Replaces the loose numeric match with explicit text assertions for singular (1), plural (2), and zero-count plural (0) rendering.

workspaces/intelligent-assistant/plugins/intelligent-assistant/src/components/tests/NotebookCard.test.tsx

Documentation (1) +4 / -2
report-alpha.api.mdRefresh translation contract with pluralized resource count keys +4/-2

Refresh translation contract with pluralized resource count keys

• Replaces the single 'notebooks.documents' and 'notebook.view.documents.count' entries with '_one'/'_other' variants in the published translation ref type contract.

workspaces/intelligent-assistant/plugins/intelligent-assistant/report-alpha.api.md

Other (6) +24 / -12
de.tsAdd German '_one'/'_other' keys for resource counts +4/-2

Add German '_one'/'_other' keys for resource counts

• Introduces singular/plural variants for both the notebook card and notebook view resource count strings.

workspaces/intelligent-assistant/plugins/intelligent-assistant/src/translations/de.ts

es.tsAdd Spanish '_one'/'_other' keys for resource counts +4/-2

Add Spanish '_one'/'_other' keys for resource counts

• Introduces singular/plural variants for both the notebook card and notebook view resource count strings.

workspaces/intelligent-assistant/plugins/intelligent-assistant/src/translations/es.ts

fr.tsAdd French '_one'/'_other' keys for resource counts +4/-2

Add French '_one'/'_other' keys for resource counts

• Introduces singular/plural variants for both the notebook card and notebook view resource count strings.

workspaces/intelligent-assistant/plugins/intelligent-assistant/src/translations/fr.ts

it.tsAdd Italian '_one'/'_other' keys for resource counts +4/-2

Add Italian '_one'/'_other' keys for resource counts

• Introduces singular/plural variants for both the notebook card and notebook view resource count strings.

workspaces/intelligent-assistant/plugins/intelligent-assistant/src/translations/it.ts

ja.tsAdd Japanese '_one'/'_other' keys for resource counts +4/-2

Add Japanese '_one'/'_other' keys for resource counts

• Adds plural key variants for consistency; both forms render the same Japanese string, matching language conventions.

workspaces/intelligent-assistant/plugins/intelligent-assistant/src/translations/ja.ts

ref.tsUpdate reference locale to pluralized resource count keys +4/-2

Update reference locale to pluralized resource count keys

• Replaces the non-pluralized resource count messages with '_one' and '_other' forms for both the notebook card and notebook view strings.

workspaces/intelligent-assistant/plugins/intelligent-assistant/src/translations/ref.ts

@rhdh-qodo-merge

rhdh-qodo-merge Bot commented Aug 7, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (1) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Inline .replace() in NotebookSurfacePage 📘 Rule violation ⚙ Maintainability
Description
formatNotebookCardDocumentsSummary() performs placeholder substitution via inline
.replace('{{count}}', ...) in an e2e page object. This violates the requirement to use a shared
replaceTemplate-style helper for {{...}} substitution, and can break when placeholder
order/occurrence differs across locales.
Code

workspaces/intelligent-assistant/e2e-tests/pages/NotebookSurfacePage.ts[353]

+    return (this.t[key] as string).replace('{{count}}', String(documentCount));
Relevance

●●● Strong

Repo tends to accept i18n/template-substitution standardization; inline string replacement is
commonly flagged for robustness.

PR-#3446
PR-#4170

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 2646 requires using a replaceTemplate-style helper for {{key}} placeholder
substitution in e2e tests rather than inline .replace(). The changed code in
NotebookSurfacePage.ts uses inline .replace('{{count}}', ...), and the existing helper
substituteNotebookTemplate() demonstrates the expected helper-based approach.

Rule 2646: Use replaceTemplate helper for template string placeholder substitution in e2e tests
workspaces/intelligent-assistant/e2e-tests/pages/NotebookSurfacePage.ts[349-353]
workspaces/intelligent-assistant/e2e-tests/utils/notebookTranslation.ts[17-25]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`formatNotebookCardDocumentsSummary()` in an e2e page object substitutes `{{count}}` using inline string `.replace()`, which is disallowed by compliance guidance for template placeholders in e2e tests.

## Issue Context
The repo already contains an e2e helper for replacing `{{placeholders}}` (`substituteNotebookTemplate`) that should be used instead of ad-hoc `.replace()`.

## Fix Focus Areas
- workspaces/intelligent-assistant/e2e-tests/pages/NotebookSurfacePage.ts[349-353]
- workspaces/intelligent-assistant/e2e-tests/utils/notebookTranslation.ts[17-25]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

2. E2E translation lookup cast 🐞 Bug ⚙ Maintainability
Description
formatNotebookCardDocumentsSummary indexes the translations object with a dynamically-typed string
and casts the result to string before calling .replace. This bypasses TypeScript’s ability to
enforce that only valid translation keys are used, reducing safety if keys change or a locale bundle
becomes incomplete.
Code

workspaces/intelligent-assistant/e2e-tests/pages/NotebookSurfacePage.ts[R349-353]

+    const key =
+      documentCount === 1
+        ? 'notebooks.documents_one'
+        : 'notebooks.documents_other';
+    return (this.t[key] as string).replace('{{count}}', String(documentCount));
Relevance

●● Moderate

Mixed precedent: team sometimes tightens TS typing, but has also rejected dynamic-key typing nits in
e2e code.

PR-#1830
PR-#3245

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new helper computes a string key and then casts the indexed translation lookup to a string,
which prevents the LightspeedMessages type (derived from the canonical message catalog) from
enforcing key correctness at compile time.

workspaces/intelligent-assistant/e2e-tests/pages/NotebookSurfacePage.ts[344-354]
workspaces/intelligent-assistant/e2e-tests/utils/translations.ts[17-46]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The e2e helper builds a key as a generic `string` and uses `(this.t[key] as string)` to appease TypeScript. This removes compile-time checking that the key is one of the known translation keys and that the value is present.

## Issue Context
`LightspeedMessages` is derived from `typeof intelligentAssistantMessages`, so TypeScript can validate lookups if the index key is a literal union. The current implementation defeats that by using a broad `string` and an `as string` cast.

## Fix Focus Areas
- workspaces/intelligent-assistant/e2e-tests/pages/NotebookSurfacePage.ts[344-354]
- workspaces/intelligent-assistant/e2e-tests/utils/translations.ts[27-46]

## Suggested implementation
1. Type the key as a literal union:
  ```ts
  const key: 'notebooks.documents_one' | 'notebooks.documents_other' =
    documentCount === 1 ? 'notebooks.documents_one' : 'notebooks.documents_other';
  ```
2. Avoid casting the lookup result; it should become `string` once the key is narrowed.
3. (Optional) Use a global replace for consistency with other helpers:
  ```ts
  return this.t[key].replace(/\{\{count\}\}/g, String(documentCount));
  ```

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Translation typing bypassed 🐞 Bug ⚙ Maintainability
Description
NotebookCard and DocumentSidebar now cast the typed translation function to Function to call
pluralized base keys (notebooks.documents, notebook.view.documents.count) that no longer exist
in the typed message catalog. This removes compile-time validation of the key and its params and
makes future translation ref changes easier to break without TypeScript catching it.
Code

workspaces/intelligent-assistant/plugins/intelligent-assistant/src/components/notebooks/NotebookCard.tsx[R208-210]

+              {(t as Function)('notebooks.documents', {
+                count: notebook.document_count ?? 0,
+              })}
Relevance

● Weak

Close precedent rejected avoiding (t as Function); team has accepted this cast pattern in
intelligent-assistant code.

PR-#3672
PR-#3837

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The translation function is explicitly typed to the translation ref’s message keys, but the base
pluralization keys were removed from the catalog and replaced by _one/_other keys. The cast is
therefore required to compile, which drops the intended key/param type checking.

workspaces/intelligent-assistant/plugins/intelligent-assistant/src/components/notebooks/NotebookCard.tsx[41-50]
workspaces/intelligent-assistant/plugins/intelligent-assistant/src/components/notebooks/NotebookCard.tsx[206-211]
workspaces/intelligent-assistant/plugins/intelligent-assistant/src/components/notebooks/DocumentSidebar.tsx[236-241]
workspaces/intelligent-assistant/plugins/intelligent-assistant/src/translations/ref.ts[24-65]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`intelligentAssistantMessages` no longer contains the base keys `notebooks.documents` / `notebook.view.documents.count`, but components still call those base keys for pluralization. This forces `(t as Function)(...)` casts, bypassing translation key/param type checking.

## Issue Context
Backstage/i18n pluralization commonly calls the *base* key with `{ count }`, and resolves to `_one`/`_other` at runtime. With the current message catalog containing only the suffixed keys, the base key is not part of `TranslationFunction<typeof intelligentAssistantTranslationRef.T>` anymore, so callers must cast.

## Fix Focus Areas
- workspaces/intelligent-assistant/plugins/intelligent-assistant/src/translations/ref.ts[24-65]
- workspaces/intelligent-assistant/plugins/intelligent-assistant/src/translations/de.ts[232-266]
- workspaces/intelligent-assistant/plugins/intelligent-assistant/src/translations/es.ts[229-262]
- workspaces/intelligent-assistant/plugins/intelligent-assistant/src/translations/fr.ts[231-266]
- workspaces/intelligent-assistant/plugins/intelligent-assistant/src/translations/it.ts[229-263]
- workspaces/intelligent-assistant/plugins/intelligent-assistant/src/translations/ja.ts[225-259]
- workspaces/intelligent-assistant/plugins/intelligent-assistant/src/components/notebooks/NotebookCard.tsx[204-212]
- workspaces/intelligent-assistant/plugins/intelligent-assistant/src/components/notebooks/DocumentSidebar.tsx[236-241]

## Suggested implementation
Choose one of the following (A is preferred for type-safety):

**A) Add base keys back as typed aliases (recommended)**
1. Add the base keys back to `intelligentAssistantMessages` (and each locale bundle), e.g.:
  - `notebooks.documents`: `'{{count}} Resources'`
  - `notebook.view.documents.count`: `'{{count}} Resources'`
  (Keep the `_one`/`_other` entries for correct plural forms.)
2. Update `NotebookCard`/`DocumentSidebar` to call `t('notebooks.documents', { count })` and `t('notebook.view.documents.count', { count })` without casting.

**B) Avoid base keys entirely**
Compute the `_one`/`_other` key in code and call `t('..._one' | '..._other', { count })` so the call remains fully typed (no cast).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context used
⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials
✅ Compliance rules (platform): 11 rules
✅ Cross-repo context
  Not relevant to this PR: redhat-developer/rhdh
  Not relevant to this PR: redhat-developer/rhdh-chart
  Not relevant to this PR: redhat-developer/rhdh-operator
  Not relevant to this PR: redhat-developer/rhdh-local

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

@rhdh-qodo-merge rhdh-qodo-merge Bot added documentation Improvements or additions to documentation Tests Bug fix labels Aug 7, 2026
@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.33%. Comparing base (3d1d7d7) to head (041e23d).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4208   +/-   ##
=======================================
  Coverage   58.33%   58.33%           
=======================================
  Files        2432     2432           
  Lines       96776    96776           
  Branches    26923    26932    +9     
=======================================
  Hits        56457    56457           
  Misses      40125    40125           
  Partials      194      194           
Flag Coverage Δ *Carryforward flag
adoption-insights 84.55% <ø> (ø) Carriedforward from 4dbe97d
ai-integrations 69.76% <ø> (ø) Carriedforward from 4dbe97d
app-defaults 69.79% <ø> (ø) Carriedforward from 4dbe97d
augment 46.67% <ø> (ø) Carriedforward from 4dbe97d
boost 76.77% <ø> (ø) Carriedforward from 4dbe97d
bulk-import 72.79% <ø> (ø) Carriedforward from 4dbe97d
cost-management 13.55% <ø> (ø) Carriedforward from 4dbe97d
dcm 67.21% <ø> (ø) Carriedforward from 4dbe97d
extensions 56.59% <ø> (ø) Carriedforward from 4dbe97d
global-floating-action-button 71.18% <ø> (ø) Carriedforward from 4dbe97d
global-header 66.50% <ø> (ø) Carriedforward from 4dbe97d
homepage 47.59% <ø> (ø) Carriedforward from 4dbe97d
install-dynamic-plugins 59.95% <ø> (ø) Carriedforward from 4dbe97d
intelligent-assistant 75.24% <100.00%> (ø)
konflux 91.98% <ø> (ø) Carriedforward from 4dbe97d
lightspeed 69.02% <ø> (ø) Carriedforward from 4dbe97d
mcp-integrations 83.40% <ø> (ø) Carriedforward from 4dbe97d
orchestrator 66.91% <ø> (ø) Carriedforward from 4dbe97d
quickstart 63.74% <ø> (ø) Carriedforward from 4dbe97d
sandbox 79.56% <ø> (ø) Carriedforward from 4dbe97d
scorecard 86.17% <ø> (ø) Carriedforward from 4dbe97d
theme 88.77% <ø> (ø) Carriedforward from 4dbe97d
translations 5.12% <ø> (ø) Carriedforward from 4dbe97d
x2a 79.20% <ø> (ø) Carriedforward from 4dbe97d

*This pull request uses carry forward flags. Click here to find out more.


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d1d7d7...041e23d. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…ok page object

French CLDR treats count=0 as singular, so the naive `=== 1` check
produced the wrong expected string for the fr locale e2e run.

Co-authored-by: Cursor <cursoragent@cursor.com>
@sonarqubecloud

sonarqubecloud Bot commented Aug 8, 2026

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant