Release 0.68.0#3339
Merged
Merged
Conversation
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* adding cutoff to serializer * adding param to search method * refactor of sync search view * ensuring all results are returned if there is a query with score_cutoff * ensuring all results are returned if there is a query with score_cutoff * spec update * adding frontend changes * fix test * fix tests * introduce manual aggregation method * re-introduce spacing at bottom * fix facet behavior * fix lint * fix lint * adding docstring * refactor to not use pandas * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Fix facet sorting Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * update spec * style lint * avoid call to aggregations * move import to top * adding test for aggregation buckets * only include the threshold if greater than 0 * refactor min score default values. add min score to settings * enforce limit and remove count call to fetch total * add max limit setting * add test * udpate spec * fix test after refactor * set default to min score * fix test * spec update * fix test * fix sorting * moving min score to constant --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…ls (/courses/o/*) (#3263)
* consolidate translations card data fetching to a single view model function * just kidding, slot is fine terminology I guess * guard against the run missing on the enrollment * use existing getNativeLanguageName function which has static fallback in case Intl isn't available. * consistently order language options * refactor getDistinctDashboardLanguageOptions * delete dead language-option helpers and singular getDashboardLanguageOptions The flat rewrite of getDistinctDashboardLanguageOptions left the older singular getDashboardLanguageOptions and getDistinctLanguageOptions without production callers. Sweep them and the helpers they kept alive. dashboardViewModel.ts: - delete getDashboardLanguageOptions (singular variant — no production caller; the picker is rendered once per dashboard, not per slot) - delete getLanguageOptionFromEnrollment, getLanguageOptionKeyValue, enrollmentMatchesCourse (only used by the singular variant) - drop getDistinctLanguageOptions from the import block languageOptions.ts: - delete getDistinctLanguageOptions (no callers after the rewrite, just its own tests) - delete getLanguageOptionLabel (only used by getDistinctLanguageOptions) - delete getLanguageCodeFromOptionKey (pure dead code, no callers anywhere — written speculatively, never wired up) - unexport getLanguageOptionKey (still used internally five times, no external consumer) getEnrollableLanguageOptions and isLanguageOptionEnrollable stay; both remain load-bearing through getDefaultLanguageOptionKey and getSelectedLanguageOption, which resolveSlotForLanguage consumes. Full deletion of languageOptions.ts is committed to Phase 7 anyway. Test changes: - delete the singular-variant tests in dashboardViewModel.test.ts; adapt the union and sort assertions to call the plural getDistinctDashboardLanguageOptions with a one-element course list - delete the getDistinctLanguageOptions tests in languageOptions.test.ts - port the three Intl-fallback / memoization tests to call getNativeLanguageName directly instead of going through the now-dead getDistinctLanguageOptions, preserving edge-case coverage at ~10 lines each instead of ~60 All 28 viewModel + languageOptions tests pass; 68 dashboard render tests still pass. Coverage of languageOptions.ts: 78% -> 91%; coverage of dashboardViewModel.ts: 74% -> 96%. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * plan + languageOptions docstring: capture phase-2 retro lessons Phase 2 retro surfaced two recurring failure modes that the existing working agreement didn't activate against: - Dead code accumulating because cleanup was deferred to "later" (singular getDashboardLanguageOptions, getLanguageCodeFromOptionKey, getLanguageOptionLabel) - Composing existing helpers without auditing their premise (isLanguageOptionEnrollable filtering against missing data — the courseruns vs language_options invariant) Plan changes (working agreement): - Add "Discover dead code and bad assumptions as you go, not at the boundary" subsection between the success criterion and the phase-boundary review questions. Three inline rules: check for remaining callers when you stop calling something; check whether an existing helper already covers the case before adding one; check the premise of helpers you compose, not just their names. - The rule lives between the during-execution norms and the phase-exit ritual to make discovery part of the moment-to-moment process, not just an end-of-phase review item. languageOptions.ts docstring: - Names the load-bearing data invariant for this file: courseruns and language_options are mostly disjoint; language_options is intent, courseruns only materializes default-language runs. - Stops short of prescribing what to do about it — the fact alone is what was missing during phase 2; readers can apply skepticism themselves. - When languageOptions.ts is absorbed into dashboardViewModel.ts in phase 7, this invariant should travel with the synthesis logic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Chris Chudzicki <christopher.chudzicki@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
OpenAPI Changes1 changes: 0 error, 0 warning, 1 info Unexpected changes? Ensure your branch is up-to-date with |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+850
to
+853
| ? getVectorClientAggregations( | ||
| allResults, | ||
| allParams, | ||
| allParams.aggregations, |
There was a problem hiding this comment.
Bug: A TypeError will crash the app in getVectorClientAggregations because it does not handle cases where the aggregationNames parameter is undefined.
Severity: CRITICAL
Suggested Fix
In SearchDisplay.tsx, provide a default empty array to getVectorClientAggregations if allParams.aggregations is undefined. For example: getVectorClientAggregations(allResults, allParams, allParams.aggregations ?? []).
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: frontends/main/src/page-components/SearchDisplay/SearchDisplay.tsx#L850-L853
Potential issue: In `SearchDisplay.tsx`, when client-side filtering is active for vector
search results (`hasClientFilters` is true), the `getVectorClientAggregations` function
is called. It receives `allParams.aggregations` as its third argument. However,
`allParams.aggregations` can be `undefined` if no aggregations are specified in the
search parameters. The `getVectorClientAggregations` function does not check for this
and attempts to call `.map()` on the `aggregationNames` argument, which will result in a
`TypeError` and crash the application if the argument is `undefined`.
Did we get this right? 👍 / 👎 to inform future reviews.
odlbot
added
waiting for checkboxes
all checkboxes checked
and removed
deploying to rc
waiting for checkboxes
labels
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
5 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.
Anastasia Beglova
Carey P Gumaer
zawan-ila
Shankar Ambady
renovate[bot]