Skip to content

Filter and order glosses/definitions by project analysis languages#4181

Draft
Copilot wants to merge 6 commits intomasterfrom
copilot/update-glosses-order-frontend
Draft

Filter and order glosses/definitions by project analysis languages#4181
Copilot wants to merge 6 commits intomasterfrom
copilot/update-glosses-order-frontend

Conversation

Copy link
Contributor

Copilot AI commented Feb 24, 2026

  • Revert fontContext.ts, fontContext.test.ts, package-lock.json, SidebarDragSense.tsx
  • SenseCardText and SensesTextSummary use useAppSelector with shallowEqual for analysisLangs
  • Removed languages prop from SenseCardContent, SenseCardText, SenseCard, WordCard
  • Removed analysisLangs selector from DragSense
  • Updated EditSensesCardContent.test.tsx with Redux Provider
  • Replaced arraysEqual with shallowEqual in DragSense and deleted the local arraysEqual function
  • Fixed prettier formatting in WordCard/index.tsx, SenseCard.tsx, SensesTextSummary.tsx
Original prompt

This section details on the original issue you should resolve

<issue_title>Display of glosses/definitions not affected by project analysis langs</issue_title>
<issue_description>In Review Entries (and possibly elsewhere), the gloss cell is a concatenation of all glosses, grouped by language, but otherwise appearing in the order they are stored in the word. (Likewise for the definitions.) Ideally:

  • They would show up in the same order as the project analysis languages
  • Any in other languages wouldn't show up at all

Q: Should this be handled in the frontend? Or should words fetched from the database have the content sorted in the backend according to the project settings?
A: Probably in the frontend, so various things that fetch the whole frontier aren't slowed down. In the frontend, we can modify and use the FontContext to get the analysis language order.

TODO:

  • Modify FontContext to hold the list of language tags for project analysis languages
  • In ReviewEntriesTable: modify the gloss cell and definition cell to only show the content that matches analysis languages, and in the same order
  • In MergeDragDrop: modify display of glosses and definitions to only show the content that matches analysis languages, and in the same order</issue_description>

Comments on the Issue (you are @copilot in this section)


🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.


This change is Reviewable

Summary by CodeRabbit

  • Refactor

    • Improved language handling across components by centralizing language configuration through application state, replacing individual prop-based language passing for more consistent behavior.
  • Tests

    • Updated test setup to properly render components within the application's state management context.

Co-authored-by: imnasnainaec <6411521+imnasnainaec@users.noreply.github.com>
Copilot AI changed the title [WIP] Update glosses/definitions display order based on project analysis languages Filter and order glosses/definitions by project analysis languages Feb 24, 2026
Copy link
Collaborator

@imnasnainaec imnasnainaec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot

  • Revert the changes to fontContext
  • Remove the languages prop from both SenseCardContent and SenseCardText
  • Move the analysisLangs = useAppSelector out of DragSense and into both SenseCardText and SensesTextSummary
  • Update tests accordingly
  • Revert the changes to package-lock.json

…ve languages prop

Co-authored-by: imnasnainaec <6411521+imnasnainaec@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 26, 2026

@copilot

  • Revert the changes to fontContext...

Done in 9177e4b. Here's a summary of the changes:

  • Reverted fontContext.ts, fontContext.test.ts, package-lock.json, and SidebarDragSense.tsx
  • SenseCardText and SensesTextSummary now use useAppSelector (with shallowEqual) to get analysisLangs directly from Redux state
  • Removed the languages prop from SenseCardContent, SenseCardText, SenseCard, and WordCard
  • Removed the analysisLangs selector from DragSense (no longer needed since SenseCardText handles it internally)
  • Updated EditSensesCardContent.test.tsx to wrap with a Redux Provider (needed since SensesTextSummary now uses Redux)

imnasnainaec

This comment was marked as resolved.

Co-authored-by: imnasnainaec <6411521+imnasnainaec@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 26, 2026

@copilot If appropriate, replace the other use of arraysEqual with shallowEqualand delete arraysEqual.

Done in d1607c4. Replaced arraysEqual with shallowEqual (already imported from react-redux) and deleted the local arraysEqual function.

@github-actions
Copy link

github-actions bot commented Feb 26, 2026

⚠️ Commit Message Format Issues ⚠️
commit 9177e4b08d:
1: T1 Title exceeds max length (86>72): "Refactor: use useAppSelector in SenseCardText/SensesTextSummary, remove languages prop"

@imnasnainaec

This comment was marked as resolved.

Co-authored-by: imnasnainaec <6411521+imnasnainaec@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 26, 2026

@copilot npm run fmt-frontend-check fails.

Fixed in b53f3d2. Ran prettier on the 3 affected files (WordCard/index.tsx, SenseCard.tsx, SensesTextSummary.tsx).

@imnasnainaec

This comment was marked as resolved.

@coderabbitai

This comment was marked as outdated.

@coderabbitai
Copy link

coderabbitai bot commented Feb 26, 2026

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The pull request removes the languages prop from multiple component interfaces and replaces prop-based language passing with Redux state derivation via useAppSelector. Components now fetch analysis languages directly from the store, enabling consistent language-aware filtering and ordering of glosses and definitions across the UI.

Changes

Cohort / File(s) Summary
Component Props Cleanup
src/components/WordCard/SenseCard.tsx, src/components/WordCard/index.tsx, src/goals/MergeDuplicates/MergeDupsStep/SenseCardContent.tsx
Removed optional languages?: string[] prop from component interfaces and call sites, simplifying public APIs.
Redux Integration for Analysis Languages
src/components/WordCard/SenseCardText.tsx, src/components/WordCard/SensesTextSummary.tsx
Integrated useAppSelector to derive analysis languages from Redux store, replacing prop-based language lists. Updated getSenseInLanguages to require language array and implemented language-aware filtering/ordering of glosses and definitions.
Helper Function Cleanup
src/goals/MergeDuplicates/MergeDupsStep/DragSense.tsx
Removed internal arraysEqual utility function and replaced with shallowEqual from react-redux; eliminated derived state for analysis languages.
Test Infrastructure
src/goals/ReviewEntries/ReviewEntriesTable/Cells/EditCell/tests/EditSensesCardContent.test.tsx
Updated test to render component within Redux Provider using mock store instead of bare prop-based rendering.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

🟩Low

Suggested reviewers

  • jasonleenaylor

Poem

A rabbit hops through Redux halls so green, 🐰
Where languages flow from a central store—
No props to pass, just selectors seen,
Glosses and defs align with grace once more!
Order restored, from state's serene core. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: filtering and ordering glosses/definitions by analysis languages as specified in linked issue #4145.
Linked Issues check ✅ Passed The PR implements all coding requirements from #4145: language filtering/ordering in SensesTextSummary and SenseCardText components, analysis language integration via useAppSelector, and removal of prop-based language passing.
Out of Scope Changes check ✅ Passed All changes are directly related to implementing language-aware glosses/definitions filtering and ordering. Modifications to component props, Redux integration, and utility functions are in scope with the linked issue requirements.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch copilot/update-glosses-order-frontend

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/components/WordCard/SenseCardText.tsx`:
- Around line 40-45: The mapping in getSenseInLanguages (and the similar block
at 54-62) can produce blank rows or render nothing when the analysis-language
list is empty; update getSenseInLanguages to first handle a null/empty languages
argument by returning an empty array, then map languages to
getSenseInLanguage(sense, l) but filter out any resulting SenseInLanguage
entries that have no gloss/definition/text (e.g., both gloss and definition
empty or whitespace) before returning so SenseCardText only receives non-empty
rows; apply the same guard+filter logic to the other language-mapping block
referenced (lines 54-62) so empty analysis-language state and empty-language
rows are not rendered.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5ebd071 and b53f3d2.

📒 Files selected for processing (7)
  • src/components/WordCard/SenseCard.tsx
  • src/components/WordCard/SenseCardText.tsx
  • src/components/WordCard/SensesTextSummary.tsx
  • src/components/WordCard/index.tsx
  • src/goals/MergeDuplicates/MergeDupsStep/MergeDragDrop/DragSense.tsx
  • src/goals/MergeDuplicates/MergeDupsStep/SenseCardContent.tsx
  • src/goals/ReviewEntries/ReviewEntriesTable/Cells/EditCell/tests/EditSensesCardContent.test.tsx

@codecov
Copy link

codecov bot commented Feb 27, 2026

Codecov Report

❌ Patch coverage is 75.86207% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.94%. Comparing base (5ebd071) to head (3b71806).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/components/WordCard/SensesTextSummary.tsx 68.18% 5 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4181      +/-   ##
==========================================
- Coverage   74.95%   65.94%   -9.01%     
==========================================
  Files         302      246      -56     
  Lines       11090     6249    -4841     
  Branches     1394      791     -603     
==========================================
- Hits         8312     4121    -4191     
+ Misses       2378     1870     -508     
+ Partials      400      258     -142     
Flag Coverage Δ
backend ?
frontend 65.94% <75.86%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Display of glosses/definitions not affected by project analysis langs

2 participants