Release 0.76.1 - #3702
Open
odlbot wants to merge 13 commits into
Open
Conversation
Next.js's default trailingSlash: false means the canonical channel route has no trailing slash, so every sitemap/API-driven request using channel_url paid for a wasted 308 redirect round-trip. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* fix: Show error message when a podcast episode fails to play --------- Co-authored-by: Ahtesham Quraish <ahtesham.quraish@192.168.1.217>
Write the comment body to a file and post via body-path instead of inlining it into the body: field, mirroring mitodl/mitxonline#3796. A large body inlined into the JS action's body: input becomes a huge INPUT_BODY env var that can exceed the OS argv+envp size limit. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* feat: identify posthog persons by keycloak global_id, not django pk This posthog project is shared with other MIT applications, and mitxpro identifies people by its own integer user ids, so Learn user N and xPro user N are currently the same posthog person. Switches both places this app names a person -- the frontend identify() call and the server-side account_created event -- to the Keycloak global_id. Users with no global_id are left unidentified rather than falling back to the pk. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: use renderWithProviders in the posthog identifier test Per review feedback. Omits the `user` option so the user still arrives via the mocked request rather than a pre-seeded query cache, keeping the pending-then-resolved sequence the app actually goes through. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Defer AI library imports to avoid eager-loading on every process litellm, langchain_litellm, tiktoken, opendataloader_pdf/cv2, and langchain_text_splitters were imported at module scope in tasks.py, etl/utils.py, and vector_search/utils.py. Those modules sit on the import chain for ordinary web request handling (views.py, learning_resources_search/api.py, vector_search/views.py), so every Django process paid the import cost even though the majority of code-paths never touch summarization, PDF-to-markdown conversion, or embedding chunking. Move each import to its single call site instead. Python's root logger config still captures these libraries' logs once they're actually used, since logger creation doesn't depend on import timing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Defer remaining eager AI-library imports, fix litellm.drop_params ordering vector_search/encoders/utils.py imported tiktoken and litellm.get_model_info at module scope, and vector_search/utils.py imports that module at the top, so both packages still loaded at boot on every web/celery process despite the rest of this PR's deferral work. learning_resources/serializers.py had the same problem with langchain_text_splitters via RecursiveJsonSplitter, loaded during django.setup() through the serializer import chain. Move all three into the functions/methods that actually use them. Also set litellm.drop_params = True in the other two modules that call litellm with params that could need dropping (the OCR batch_completion call and the embedding call), rather than relying on content_summarizer.py -- now itself lazily imported -- to have set it first. get_model_info call sites don't send params, so they're unaffected either way. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Disable sentry_sdk auto-enabling integrations to stop langchain boot import sentry_sdk.init() auto-detects installed packages and eagerly imports each auto-enabling integration module to check applicability -- including langchain, whose Sentry integration module unconditionally imports langchain_core at module scope. langchain_core is a transitive dependency of litellm/langchain-litellm, never used directly, but importing it (and the tiktoken/langchain_text_splitters it pulls in) undid the startup-cost work in the rest of this PR regardless of any of our own import sites. sentry_sdk has no way to disable a single named auto-enabling integration without importing it first (disabled_integrations requires an instance), so auto_enabling_integrations=False is the only way to skip the import. That drops all of sentry's auto-detected integrations, not just langchain; of that list only Redis applies to this app, and redis is already loaded at boot via Celery/the cache backend, so it's added back explicitly at no additional import cost. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix encoders/utils_test.py patch targets for the deferred imports CI caught what local runs missed: this test file patched vector_search.encoders.utils.get_model_info/.tiktoken as module attributes, which broke once those imports moved inside truncate_to_model_limit in 674f782. Patch the source modules (litellm.get_model_info, tiktoken.get_encoding) instead, which the function's local imports pick up correctly either way. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Add Boto3/Httpx sentry integrations, regression-test boot imports Both boto3 and httpx already load unconditionally at boot regardless of Sentry (boto3 via learning_resources.urls, httpx via Django admin autodiscover), confirmed with the same sys.modules probe used to find the langchain issue -- so enabling their Sentry integrations explicitly adds no import cost, per review feedback. Also add a regression test per review request: boots Django and serves a liveness request in a fresh subprocess (so nothing is already cached from other tests), then asserts none of the packages this PR deferred (tiktoken, litellm, langchain_core/classic/text_splitters, opendataloader_pdf, cv2, pdf2image) show up in sys.modules. Verified it actually catches a regression by reverting the sentry auto_enabling_integrations fix and the serializers.py RecursiveJsonSplitter deferral in turn -- both make the test fail as expected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* feat(b2b): org analytics dashboard for the manager surface Adds the reporting half of the B2B org-manager dashboard, alongside the existing seat-administration half (ContractAdminPage): contract KPI cards, a monthly engagement trend, per-course-run performance, and a program funnel, each backed by one of ol-analytics-api's org-scoped materialized views. Keyed on the Keycloak org UUID (sso_organization_id), not the org slug or MITx Online's numeric id — that is the only identifier stable across the JWT, MITx Online and StarRocks, and the one the analytics API filters on (see mitodl/ol-analytics-api#13, mitodl/mitxonline#3789). The published mitxonline client does not declare the field yet, so it is read off the wire through a shim that returns null when absent; the dashboard reports itself unavailable rather than issuing a request with `undefined` in the path. Charts use @mui/x-charts rather than recharts. x-charts reads the existing MUI v6 theme directly, so axis, tooltip and legend styling comes from the same tokens as the rest of the app instead of being threaded through props by hand; it also avoids pulling Redux, immer and a second lodash-alike into the bundle, which recharts 3 ships as runtime dependencies. Chart colors are MIT Learn tokens checked against the data-viz palette rules (lightness band, chroma floor, CVD separation, contrast) rather than picked by eye — two slots are stepped versions of brand hues that fall outside the readable band. Numbers the API withholds under its k-anonymity floor arrive as null. They are rendered as an explained placeholder, never coerced to zero: a suppressed month is a gap in the trend line, and the program funnel is paired with a table so a withheld stage can say so instead of just missing a bar. Auth stays cookie-based; the analytics API sits behind APISIX, which mints the JWT from the session. NEXT_PUBLIC_ANALYTICS_API_BASE_URL is optional so environments without the service still boot. Also extracts the contract admin page's table primitives into components/B2BTable so both surfaces share them, and polyfills structuredClone in the jest setup — jsdom lacks it and x-charts requires it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SjK3vzqB6nvLHKUF7686Pj * docs(env): correct what NEXT_PUBLIC_ANALYTICS_API_BASE_URL actually gates Both env comments implied the variable controls whether the analytics dashboard is visible. It does not: reachability is gated by the b2b-analytics-dashboard PostHog flag, and with the flag on and this unset the route still renders, reporting itself unavailable. The variable only decides whether there is an API to load data from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SjK3vzqB6nvLHKUF7686Pj * fix(b2b): tell a failed analytics section apart from an empty one Each section only received rows/asOf/isLoading, all falsy on a failed query, so a section whose request 500'd rendered the empty state: "No course enrollments recorded yet" plus "Data not yet refreshed". Both are claims — one about the org, one about the materialized view — that a failed request gives no grounds for. Threading isError through lets each section say what is actually known: nothing. Pairs the engagement line chart with a table of the same monthly numbers, mirroring ProgramFunnelChart. An SVG of plotted geometry is unreadable to a screen reader, and a suppressed month is drawn as a gap that reads identically to "no data" — only the table can say it was withheld. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(b2b): say when an analytics section is showing a subset Every endpoint is paged, and a truncated page is invisible in the rows themselves: 2 rows of 340 look exactly like 2 of 2. An org past the cap got a dashboard that read as complete while quietly dropping the rest. Consumes the envelope's new `total_count` (mitodl/ol-analytics-api#15) to compare what each section rendered against what the org actually has, and offers the remainder in one page. The ask is capped at the API's own max_page_size, since it answers 422 above that — past the cap the message stands alone rather than offering a button that cannot deliver. Per-section limits, so expanding a long course table doesn't refetch the other three, and keepPreviousData so the section a manager just expanded doesn't blank back to its skeleton on the way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(b2b): don't offer "Show all" when it would change nothing The API applies its LIMIT in SQL and drops sub-floor rows in Python afterwards, so a page of `total_count` rows can still come back short — ask for 340 and get 328, permanently. `canShowAll` was gated only on the API's page cap, so in that case the button stayed, and clicking it resent the identical limit: same query key, no refetch, nothing happened. Gate on whether the click would actually raise the limit instead. Covers both dead-end cases (already at the cap, already asked for the total) and leaves the count standing on its own, which is the honest reading — the remainder is not reachable through this endpoint at that grain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(b2b): read chart chrome from the theme instead of copied hexes CHART_INK pinned four hexes whose own trailing comments named the tokens they were copied from. Retuning any of those tokens in smoot-design would have left the chart grid, axes and labels behind while the tables beside them moved — a silent divergence with nothing to catch it. Now a chartInk(theme) lookup. The values are identical today (verified against the token set), so this is a pure refactor. The two data palettes stay pinned deliberately, and the module says why: they are validated for contrast and CVD separation, so they must not move without re-running that check. Non-data ink is the opposite case — it should track the design system, not resist it. Adds useTheme to ol-components' MUI re-exports, alongside styled and alpha. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(b2b): make expanding a section perceivable, and actually keep the rows Two problems behind one review comment about "Show all" giving no feedback. The first is that `placeholderData: keepPreviousData` was silently inert under `useQueries`: on a limit change the result went straight to `data: undefined`, so the section blanked back to its skeleton — the exact thing the option was there to prevent, and the opposite of what the code claimed. It behaves as documented on `useQuery`, so the four queries are now four `useQuery` calls (fixed count, fixed order, so hook-safe). The second is the reported one. With the rows correctly retained, nothing observable changes on click, so the control now reports its own state: aria-busy and aria-disabled while the expanded page is in flight, label "Loading…", and the result announced through a polite live region. Follows the CSV export button on ContractAdminPage. aria-disabled rather than disabled so a keyboard user does not lose focus mid-action. The live region sits outside the truncation message rather than inside it: when expanding completes the section, both message and button unmount, and a region that unmounts in the same commit as the change it describes announces nothing. The announcement is derived from render state, not detected as an edge in an effect. An edge detector has to observe the in-flight render to arm itself, and React can batch a fast resolution into one commit — on a warm cache it would simply never fire. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(b2b): hide the analytics charts from assistive tech Both charts are paired with a table carrying the same numbers, so the SVG is redundant to a screen reader — and worse than redundant: reaching the data means walking hundreds of series, axis and label nodes first, to be handed it properly a moment later. The table is the accessible copy, so the chart is now aria-hidden and the table stays exposed. Checked before hiding that neither chart renders anything focusable, since aria-hidden over focusable content is its own violation — keyboard focus can land on a node screen readers were told does not exist. A test asserts both halves, so this fails if a future @mui/x-charts starts emitting focusable nodes rather than silently becoming a keyboard trap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* Readd dropped GTM calls * Fix tests
…3697) InfoBoxCourse's session-switch tests pick an option with a regex built from one run's start date, but only pinned `start_date` — the factory filled `end_date` with `faker.date.future()`. Options are labelled with the whole range ("Aug 28 - Sep 27, 2026"), so whenever a run's random end date landed on another run's start date, the regex matched two options and the suite failed with "Found multiple elements". That is what took down javascript-tests on an unrelated PR: a run starting Aug 28 drew the end date Sep 27, colliding with the Sep 27 start of the run the test was clicking. Roughly a 1-in-365 draw, so it surfaces at random on whichever PR happens to be unlucky. Pins both ends of both runs, spaced so no two labelled dates can share a calendar day. Confirmed the mechanism first by reproducing it deliberately (end date set equal to the next run's start reproduces the exact error) and then confirming the chosen spacing keeps the query unique. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
OpenAPI ChangesNo changes detected 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
+268
to
+269
| // rather than stored, since no load is ever attempted. | ||
| const error = hasAudioSource ? loadError : PLAYBACK_ERROR_MESSAGES.noSource |
There was a problem hiding this comment.
Bug: A race condition in the retry logic causes the buffering state to be prematurely set to false because the onError handler is incorrectly triggered by an abort event.
Severity: MEDIUM
Suggested Fix
Update the onError handler to avoid calling setIsBuffering(false) when the error code is MEDIA_ERR_ABORTED. This type of event is a normal part of the audio.load() process and should not clear the buffering state, which is meant to indicate that a new source is loading.
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/app-pages/PodcastPage/useAudioPlayer.ts#L268-L269
Potential issue: A race condition exists in the playback retry logic. When a user
retries a failed playback, the `retry()` function sets `isBuffering` to `true` and calls
`audio.load()`. This `audio.load()` call triggers an asynchronous `abort` event, which
is handled by the `onError` callback. The `onError` handler then immediately sets
`isBuffering` back to `false`. This prematurely clears the buffering state before the
new audio source has actually loaded, causing the buffering indicator to flicker and
potentially leading to incorrect focus management in `usePlaybackRecovery`, which
depends on an accurate buffering state.
Did we get this right? 👍 / 👎 to inform future reviews.
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
9 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.
Tobias Macey
Dan Subak
Shankar Ambady
Anastasia Beglova
Chris Chudzicki
Nathan Levesque
Danielle Frappier
Ahtesham Quraish