feat: updated bulletins and new top chrome renderer#4705
Draft
cemreinanc wants to merge 6 commits intomainfrom
Draft
feat: updated bulletins and new top chrome renderer#4705cemreinanc wants to merge 6 commits intomainfrom
cemreinanc wants to merge 6 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR refactors the bulletin management system and page header architecture, moving from client-side driven header logic to a server-rendered, context-based approach with dynamic height-aware sticky positioning. The ChangesBulletin & TopChrome Architecture
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Server
participant DB
participant Client
Browser->>Server: Request page (x-url header)
Server->>Server: resolveInitialTopChromeHeaderState()
Server->>DB: Fetch community if route is community
DB-->>Server: Community data
Server->>Server: Fetch bulletins & dismissed IDs
DB-->>Server: Bulletins[], dismissed[]
Server-->>Browser: HTML with TopChromeHeaderProvider init state
Browser->>Client: Mount TopChrome & TopChromeHeaderProvider
Client->>Client: TopChromeClient measures & writes --top-chrome-height
Client->>Client: BulletinsClient queries for fresh bulletins
Client->>DB: GET /get-bulletins/
DB-->>Client: BulletinItem[]
Client->>Client: Render first non-dismissed Bulletin
User->>Client: Click dismiss on Bulletin
Client->>Client: Serialize dismissed IDs to cookie
Client->>DB: POST /cancel-bulletin/ for each synced ID
DB-->>Client: Success
Client->>Client: Update cookie & local state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (4)
front_end/package.json (1)
127-127: 💤 Low valueMinor type version mismatch with sanitize-html.
The
@types/sanitize-htmlversion is2.16.1whilesanitize-htmlis2.17.3. While this is typically compatible, consider updating the types package to match the library version more closely to ensure complete type coverage.🤖 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 `@front_end/package.json` at line 127, Update the `@types` package to match the sanitize-html library version: change the dependency entry for "@types/sanitize-html" to a version that aligns with "sanitize-html" (e.g., bump to 2.17.x) so type definitions better match the runtime; update the package.json dependency for "@types/sanitize-html" and run the lockfile update (npm/yarn/pnpm install) to ensure the new types are installed and consistent with the "sanitize-html" package.front_end/src/app/(services-quiz)/components/services_quiz_flow_content.tsx (1)
26-27: 💤 Low valueConsider safer type narrowing instead of assertion.
The type assertion
step as ServicesQuizStepIdbypasses type checking. While the fallback?? STEP_COMPONENTS[1]handles invalid steps, consider whetherstepshould already be properly typed fromuseServicesQuizFlow().If
stepcan legitimately be outside the valid range, the current approach is acceptable. Otherwise, fixing the upstream type would eliminate the need for the assertion.🤖 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 `@front_end/src/app/`(services-quiz)/components/services_quiz_flow_content.tsx around lines 26 - 27, The code is using a type assertion on step when picking ActiveStep (ActiveStep, STEP_COMPONENTS, ServicesQuizStepId, useServicesQuizFlow) which bypasses type safety; instead, ensure step is properly typed upstream by updating useServicesQuizFlow to return ServicesQuizStepId, or validate/narrow step before indexing (e.g., check it's an integer and within STEP_COMPONENTS bounds or use a runtime existence check) and then select STEP_COMPONENTS[step] with a safe fallback, removing the unsafe "as" assertion.front_end/src/app/(main)/components/top_chrome_client.tsx (1)
42-47: ⚡ Quick winConsider adding error handling for MutationObserver.
While
ResizeObserver.observe()is wrapped in try/catch (lines 60-66), theMutationObserver.observe()call is not. Although less common,MutationObserver.observe()can throw if the target node is invalid or options are malformed.🛡️ Proposed error handling
const observer = new MutationObserver(updateTopChromeHeight); - observer.observe(topChromeEl, { - attributes: true, - childList: true, - subtree: true, - }); + try { + observer.observe(topChromeEl, { + attributes: true, + childList: true, + subtree: true, + }); + } catch (error) { + logError(error, { + message: "Failed to observe top chrome height with MutationObserver", + }); + } window.addEventListener("resize", updateTopChromeHeight);🤖 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 `@front_end/src/app/`(main)/components/top_chrome_client.tsx around lines 42 - 47, Wrap the MutationObserver.observe call in a try/catch similar to the ResizeObserver handling: when creating the observer (const observer = new MutationObserver(updateTopChromeHeight)) surround observer.observe(topChromeEl, { attributes: true, childList: true, subtree: true }) with a try block and on catch log the error (or handle it) using the same logger/cleanup pattern used for ResizeObserver to avoid uncaught exceptions if topChromeEl is invalid or options are malformed.front_end/src/hooks/use_top_chrome_height.ts (1)
35-40: ⚡ Quick winConsider defensive error handling for MutationObserver.
Similar to
top_chrome_client.tsx, theobserver.observe()call could benefit from try/catch protection, though failures are rare in practice.🛡️ Optional defensive wrapper
const observer = new MutationObserver(updateTopChromeHeight); - observer.observe(document.documentElement, { - attributes: true, - attributeFilter: ["style"], - }); + try { + observer.observe(document.documentElement, { + attributes: true, + attributeFilter: ["style"], + }); + } catch (error) { + // Log but don't fail; we've already set initial height + console.error("Failed to observe top chrome height:", error); + } window.addEventListener("resize", updateTopChromeHeight);🤖 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 `@front_end/src/hooks/use_top_chrome_height.ts` around lines 35 - 40, Wrap the MutationObserver.observe call in a try/catch to defensively handle rare failures: when creating/starting the observer (the `observer.observe(document.documentElement, { attributes: true, attributeFilter: ["style"] })` invocation in use_top_chrome_height.ts), catch any thrown error, log or handle it (e.g., console.warn or a provided logger) and avoid crashing the hook; still attach the `window.addEventListener("resize", updateTopChromeHeight)` fallback so resize updates continue, and ensure you call `observer.disconnect()` in the hook cleanup only if observation succeeded.
🤖 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 `@front_end/src/app/`(main)/components/bulletin.tsx:
- Around line 47-53: Replace the clickable FontAwesomeIcon with a real button
element and only render it when the dismissal handler exists: conditionally
render the control when onHidden is defined, use a <button> (instead of
FontAwesomeIcon alone) that contains the FontAwesomeIcon component, add
accessible attributes like aria-label and type="button" and ensure keyboard
activation calls onHidden; also replace any hardcoded English aria-label/text
with the translation string (e.g., via your existing translation
function/useTranslation) so the dismiss control is localized. Reference:
FontAwesomeIcon and onHidden in bulletin.tsx.
In `@front_end/src/app/`(main)/components/bulletins_client.tsx:
- Around line 48-55: The query function for React Query (queryFn) currently
catches errors from ClientMiscApi.getBulletins, logs them via logError, and
returns [] which overwrites valid SSR/initialData; change the catch to log the
error with logError but then rethrow (or throw the caught error) instead of
returning an empty array so React Query can preserve the last successful data
(leave ClientMiscApi.getBulletins and queryFn as the referenced symbols to
locate the code).
- Around line 76-79: The code adds bulletinId to
syncedDismissedBulletinIdsRef.current before the network call and never removes
it on failure, so transient errors permanently mark a dismiss as synced; change
the logic in the dismiss flow that uses syncedDismissedBulletinIdsRef and
dismissBulletin so the id is only added after dismissBulletin resolves
successfully, or if you must optimistically add it then remove it inside the
catch handler (using syncedDismissedBulletinIdsRef.current.delete(bulletinId))
and rethrow/log via logError to allow retries; ensure all references to
syncedDismissedBulletinIdsRef, dismissBulletin, logError, and bulletinId are
updated accordingly.
- Around line 37-42: The component seeds dismissedBulletinIds and
syncedDismissedBulletinIdsRef only on initial mount so client state doesn't
update when new initialDismissedBulletinIds/initialSyncedDismissedBulletinIds
props arrive (causing dismissal leakage across sessions); add a useEffect in the
same component (bulletins_client.tsx) that watches initialDismissedBulletinIds
and initialSyncedDismissedBulletinIds and calls setDismissedBulletinIds(() =>
new Set(initialDismissedBulletinIds)) and sets
syncedDismissedBulletinIdsRef.current = new
Set(initialSyncedDismissedBulletinIds) to reset the state/ref whenever the prop
values change.
In `@front_end/src/app/`(main)/components/bulletins_shared.ts:
- Around line 13-19: The current parsing chain (value.split(...).map(id =>
Number.parseInt(id, 10)).filter(Number.isFinite)) accepts malformed entries like
"12abc", negatives and decimals; replace it with strict positive-integer
validation: when mapping each raw id from value.split(...) validate the raw
string with a digits-only check (e.g. /^\d+$/) or use Number and
Number.isInteger plus > 0 before including it, and then dedupe as before; update
both occurrences (the shown parseInt usage and the same block at lines 23-27) to
only accept strictly positive integers and reject all other inputs.
In `@front_end/src/app/`(main)/components/content_translated_banner/index.tsx:
- Around line 56-58: The span contains a hardcoded English label "translated
by"; replace it with the i18n helper by importing and using useTranslations()
and calling t(...) where this component (e.g., the ContentTranslatedBanner
component or its functional scope) renders that span; update the JSX to use
t("translated_by") (or the appropriate key you add to your translation files) so
the string is localized and ensure the translation key exists in the locale JSON
used by useTranslations().
In `@front_end/src/app/`(main)/notebooks/components/notebook_content_sections.tsx:
- Around line 84-87: The TOC active-section offset ignores dynamic chrome height
on desktop: update the offset calculation used when computing activeId so it
includes topChromeHeight for both branches (i.e., change the isLargeScreen
branch from using DESKTOP_SCROLL_OFFSET alone to DESKTOP_SCROLL_OFFSET +
topChromeHeight or apply a bulletin delta before computing activeId). Locate the
variables and logic around offset, isLargeScreen, DESKTOP_SCROLL_OFFSET,
topChromeHeight and activeId in notebook_content_sections.tsx and ensure the
desktop path uses the real chrome height when computing the scroll offset for
active section highlighting.
In
`@front_end/src/app/`(prediction-flow)/tournament/[slug]/prediction-flow/loading.tsx:
- Around line 29-41: The loading header shows exit controls that are
non-functional; update the two Button usages inside FlowHeaderActions (the text
button that renders t("exitPredictionFlow") and the icon-only Button with
FontAwesomeIcon/faRightFromBracket) to call the real exit handler (pass the
existing exit function or navigation callback as an onClick or provide an href)
and ensure the icon-only Button has an accessible label (e.g., aria-label or
aria-labelledby) so screen readers can announce the action; keep
styling/variants the same while wiring these Buttons to the actual exit
behavior.
In `@misc/models.py`:
- Around line 47-48: The Bulletin model's __str__ currently slices raw HTML from
self.text which can leak tags; update Bulletin.__str__ to first convert/sanitize
the stored HTML to plain text (e.g., use django.utils.html.strip_tags or an
equivalent HTML-to-text helper) then truncate that plaintext to 150 characters
and append "..." when longer; add the needed import (strip_tags) and ensure you
call the plain-text variable in the return value instead of self.text.
In `@misc/views.py`:
- Around line 143-145: The code directly calls Bulletin.objects.get(pk=pk)
inside BulletinViewedBy.objects.get_or_create which raises Bulletin.DoesNotExist
and causes a 500; first resolve the Bulletin using a 404-safe lookup (e.g.,
Django's get_object_or_404 or catch Bulletin.DoesNotExist and raise Http404)
into a local variable (e.g., bulletin) and then call
BulletinViewedBy.objects.get_or_create(bulletin=bulletin, user=user) so
stale/invalid ids return a 404 instead of a server error.
---
Nitpick comments:
In `@front_end/package.json`:
- Line 127: Update the `@types` package to match the sanitize-html library
version: change the dependency entry for "@types/sanitize-html" to a version
that aligns with "sanitize-html" (e.g., bump to 2.17.x) so type definitions
better match the runtime; update the package.json dependency for
"@types/sanitize-html" and run the lockfile update (npm/yarn/pnpm install) to
ensure the new types are installed and consistent with the "sanitize-html"
package.
In `@front_end/src/app/`(main)/components/top_chrome_client.tsx:
- Around line 42-47: Wrap the MutationObserver.observe call in a try/catch
similar to the ResizeObserver handling: when creating the observer (const
observer = new MutationObserver(updateTopChromeHeight)) surround
observer.observe(topChromeEl, { attributes: true, childList: true, subtree: true
}) with a try block and on catch log the error (or handle it) using the same
logger/cleanup pattern used for ResizeObserver to avoid uncaught exceptions if
topChromeEl is invalid or options are malformed.
In `@front_end/src/app/`(services-quiz)/components/services_quiz_flow_content.tsx:
- Around line 26-27: The code is using a type assertion on step when picking
ActiveStep (ActiveStep, STEP_COMPONENTS, ServicesQuizStepId,
useServicesQuizFlow) which bypasses type safety; instead, ensure step is
properly typed upstream by updating useServicesQuizFlow to return
ServicesQuizStepId, or validate/narrow step before indexing (e.g., check it's an
integer and within STEP_COMPONENTS bounds or use a runtime existence check) and
then select STEP_COMPONENTS[step] with a safe fallback, removing the unsafe "as"
assertion.
In `@front_end/src/hooks/use_top_chrome_height.ts`:
- Around line 35-40: Wrap the MutationObserver.observe call in a try/catch to
defensively handle rare failures: when creating/starting the observer (the
`observer.observe(document.documentElement, { attributes: true, attributeFilter:
["style"] })` invocation in use_top_chrome_height.ts), catch any thrown error,
log or handle it (e.g., console.warn or a provided logger) and avoid crashing
the hook; still attach the `window.addEventListener("resize",
updateTopChromeHeight)` fallback so resize updates continue, and ensure you call
`observer.disconnect()` in the hook cleanup only if observation succeeded.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0bff60ed-86d9-430e-aec2-a2a4adfd2389
⛔ Files ignored due to path filters (1)
front_end/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (77)
front_end/package.jsonfront_end/src/app/(campaigns-registration)/(bridgewater)/bridgewater-2025/contest-rules/page.tsxfront_end/src/app/(campaigns-registration)/(bridgewater)/bridgewater-2025/how-it-works/page.tsxfront_end/src/app/(campaigns-registration)/(bridgewater)/bridgewater-2025/layout.tsxfront_end/src/app/(campaigns-registration)/(bridgewater)/bridgewater-2025/leaderboards/page.tsxfront_end/src/app/(campaigns-registration)/(bridgewater)/bridgewater-2025/notice-at-collection/page.tsxfront_end/src/app/(campaigns-registration)/(bridgewater)/bridgewater-2025/page.tsxfront_end/src/app/(campaigns-registration)/(bridgewater)/bridgewater-2025/q1/page.tsxfront_end/src/app/(campaigns-registration)/(bridgewater)/bridgewater-reg/layout.tsxfront_end/src/app/(campaigns-registration)/(bridgewater)/bridgewater-reg/page.tsxfront_end/src/app/(campaigns-registration)/(bridgewater)/bridgewater/contest-rules/page.tsxfront_end/src/app/(campaigns-registration)/(bridgewater)/bridgewater/how-it-works/page.tsxfront_end/src/app/(campaigns-registration)/(bridgewater)/bridgewater/layout.tsxfront_end/src/app/(campaigns-registration)/(bridgewater)/bridgewater/leaderboards/page.tsxfront_end/src/app/(campaigns-registration)/(bridgewater)/bridgewater/notice-at-collection/page.tsxfront_end/src/app/(campaigns-registration)/(bridgewater)/bridgewater/page.tsxfront_end/src/app/(campaigns-registration)/(bridgewater)/bridgewater/q1/page.tsxfront_end/src/app/(campaigns-registration)/id-verification/page.tsxfront_end/src/app/(campaigns-registration)/rand/confirm/page.tsxfront_end/src/app/(campaigns-registration)/rand/contest-rules/page.tsxfront_end/src/app/(campaigns-registration)/rand/layout.tsxfront_end/src/app/(campaigns-registration)/rand/page.tsxfront_end/src/app/(futureeval)/futureeval/components/futureeval-navbar.tsxfront_end/src/app/(main)/(tournaments)/tournaments/components/tournaments_header.tsxfront_end/src/app/(main)/accounts/actions.tsfront_end/src/app/(main)/actions.tsfront_end/src/app/(main)/aggregation-explorer/components/aggregation_graph_panel.tsxfront_end/src/app/(main)/c/[slug]/page.tsxfront_end/src/app/(main)/c/[slug]/settings/page.tsxfront_end/src/app/(main)/components/bulletin.tsxfront_end/src/app/(main)/components/bulletins.tsxfront_end/src/app/(main)/components/bulletins_client.tsxfront_end/src/app/(main)/components/bulletins_shared.tsfront_end/src/app/(main)/components/content_translated_banner/index.tsxfront_end/src/app/(main)/components/headers/community_header.tsxfront_end/src/app/(main)/components/headers/global_header.tsxfront_end/src/app/(main)/components/headers/header.tsxfront_end/src/app/(main)/components/impersonation_banner_client.tsxfront_end/src/app/(main)/components/impersonation_banner_server.tsxfront_end/src/app/(main)/components/top_chrome.tsxfront_end/src/app/(main)/components/top_chrome_client.tsxfront_end/src/app/(main)/components/top_chrome_error_boundary.tsxfront_end/src/app/(main)/components/top_chrome_header_context.tsxfront_end/src/app/(main)/components/top_chrome_header_server.tsfront_end/src/app/(main)/components/top_chrome_header_shared.tsfront_end/src/app/(main)/components/top_chrome_header_slot.tsxfront_end/src/app/(main)/error.tsxfront_end/src/app/(main)/labor-hub/components/labor_hub_navigation.tsxfront_end/src/app/(main)/layout.tsxfront_end/src/app/(main)/not-found.tsxfront_end/src/app/(main)/notebooks/[id]/[[...slug]]/page_compotent.tsxfront_end/src/app/(main)/notebooks/components/notebook_content_sections.tsxfront_end/src/app/(main)/questions/[id]/[[...slug]]/page_component.tsxfront_end/src/app/(main)/questions/components/sidebar.tsxfront_end/src/app/(main)/questions/create/[content_type]/page.tsxfront_end/src/app/(main)/questions/create/page.tsxfront_end/src/app/(main)/questions/page.tsxfront_end/src/app/(prediction-flow)/components/header.tsxfront_end/src/app/(prediction-flow)/tournament/[slug]/prediction-flow/loading.tsxfront_end/src/app/(prediction-flow)/tournament/[slug]/prediction-flow/page.tsxfront_end/src/app/(services-quiz)/components/services_quiz_flow_content.tsxfront_end/src/app/(services-quiz)/components/services_quiz_header.tsxfront_end/src/app/(services-quiz)/components/services_quiz_screen.tsxfront_end/src/app/(storefront)/layout.tsxfront_end/src/app/not-found.tsxfront_end/src/components/flow/flow_header.tsxfront_end/src/components/markdown_editor/editor.cssfront_end/src/components/ui/tabs/index.tsxfront_end/src/hooks/use_top_chrome_height.tsfront_end/src/services/api/misc/misc.shared.tsfront_end/src/utils/navigation.tsfront_end/tailwind.config.tsmisc/admin.pymisc/migrations/0009_remove_bulletin_post_project.pymisc/models.pymisc/urls.pymisc/views.py
💤 Files with no reviewable changes (5)
- front_end/src/app/(campaigns-registration)/(bridgewater)/bridgewater-reg/page.tsx
- front_end/src/app/(main)/components/headers/global_header.tsx
- front_end/src/utils/navigation.ts
- front_end/src/app/(campaigns-registration)/rand/page.tsx
- front_end/src/app/(campaigns-registration)/rand/confirm/page.tsx
Comment on lines
+47
to
+53
| <FontAwesomeIcon | ||
| className="absolute right-3 top-2 inline cursor-pointer hover:text-gray-700" | ||
| icon={faClose} | ||
| onClick={() => { | ||
| onHidden?.(); | ||
| }} | ||
| /> |
Contributor
There was a problem hiding this comment.
Make the dismiss control accessible and avoid a no-op close icon.
The clickable FontAwesomeIcon is not keyboard-accessible, and it still renders even when onHidden is undefined. Use a real <button> and render it only when dismissal is supported.
Suggested fix
- <FontAwesomeIcon
- className="absolute right-3 top-2 inline cursor-pointer hover:text-gray-700"
- icon={faClose}
- onClick={() => {
- onHidden?.();
- }}
- />
+ {onHidden && (
+ <button
+ type="button"
+ className="absolute right-3 top-2 inline p-0 text-inherit hover:text-gray-700"
+ onClick={onHidden}
+ aria-label="Dismiss bulletin"
+ >
+ <FontAwesomeIcon icon={faClose} className="pointer-events-none" />
+ </button>
+ )}🤖 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 `@front_end/src/app/`(main)/components/bulletin.tsx around lines 47 - 53,
Replace the clickable FontAwesomeIcon with a real button element and only render
it when the dismissal handler exists: conditionally render the control when
onHidden is defined, use a <button> (instead of FontAwesomeIcon alone) that
contains the FontAwesomeIcon component, add accessible attributes like
aria-label and type="button" and ensure keyboard activation calls onHidden; also
replace any hardcoded English aria-label/text with the translation string (e.g.,
via your existing translation function/useTranslation) so the dismiss control is
localized. Reference: FontAwesomeIcon and onHidden in bulletin.tsx.
Comment on lines
+37
to
+42
| const [dismissedBulletinIds, setDismissedBulletinIds] = useState( | ||
| () => new Set(initialDismissedBulletinIds) | ||
| ); | ||
| const syncedDismissedBulletinIdsRef = useRef( | ||
| new Set(initialSyncedDismissedBulletinIds) | ||
| ); |
Contributor
There was a problem hiding this comment.
Prop-seeded dismissal state goes stale after login/logout.
useState and useRef only read these props on the first mount. In this persistent layout, later server renders with new initialDismissedBulletinIds / initialSyncedDismissedBulletinIds won't refresh the client state, so dismissals can leak across session changes or reappear until a hard reload.
Suggested fix
const [dismissedBulletinIds, setDismissedBulletinIds] = useState(
() => new Set(initialDismissedBulletinIds)
);
const syncedDismissedBulletinIdsRef = useRef(
new Set(initialSyncedDismissedBulletinIds)
);
+
+ useEffect(() => {
+ setDismissedBulletinIds(new Set(initialDismissedBulletinIds));
+ }, [initialDismissedBulletinIds]);
+
+ useEffect(() => {
+ syncedDismissedBulletinIdsRef.current = new Set(
+ initialSyncedDismissedBulletinIds
+ );
+ }, [initialSyncedDismissedBulletinIds]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const [dismissedBulletinIds, setDismissedBulletinIds] = useState( | |
| () => new Set(initialDismissedBulletinIds) | |
| ); | |
| const syncedDismissedBulletinIdsRef = useRef( | |
| new Set(initialSyncedDismissedBulletinIds) | |
| ); | |
| const [dismissedBulletinIds, setDismissedBulletinIds] = useState( | |
| () => new Set(initialDismissedBulletinIds) | |
| ); | |
| const syncedDismissedBulletinIdsRef = useRef( | |
| new Set(initialSyncedDismissedBulletinIds) | |
| ); | |
| useEffect(() => { | |
| setDismissedBulletinIds(new Set(initialDismissedBulletinIds)); | |
| }, [initialDismissedBulletinIds]); | |
| useEffect(() => { | |
| syncedDismissedBulletinIdsRef.current = new Set( | |
| initialSyncedDismissedBulletinIds | |
| ); | |
| }, [initialSyncedDismissedBulletinIds]); |
🤖 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 `@front_end/src/app/`(main)/components/bulletins_client.tsx around lines 37 -
42, The component seeds dismissedBulletinIds and syncedDismissedBulletinIdsRef
only on initial mount so client state doesn't update when new
initialDismissedBulletinIds/initialSyncedDismissedBulletinIds props arrive
(causing dismissal leakage across sessions); add a useEffect in the same
component (bulletins_client.tsx) that watches initialDismissedBulletinIds and
initialSyncedDismissedBulletinIds and calls setDismissedBulletinIds(() => new
Set(initialDismissedBulletinIds)) and sets syncedDismissedBulletinIdsRef.current
= new Set(initialSyncedDismissedBulletinIds) to reset the state/ref whenever the
prop values change.
Comment on lines
+48
to
+55
| queryFn: async () => { | ||
| try { | ||
| return await ClientMiscApi.getBulletins(); | ||
| } catch (error) { | ||
| logError(error); | ||
| return []; | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
Don't wipe SSR bulletins on a transient refetch failure.
Returning [] here replaces valid initialData with “no bulletins” whenever the client fetch fails once. Let the query error instead so React Query keeps the last successful data in place.
Suggested fix
queryFn: async () => {
try {
return await ClientMiscApi.getBulletins();
} catch (error) {
logError(error);
- return [];
+ throw error;
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| queryFn: async () => { | |
| try { | |
| return await ClientMiscApi.getBulletins(); | |
| } catch (error) { | |
| logError(error); | |
| return []; | |
| } | |
| }, | |
| queryFn: async () => { | |
| try { | |
| return await ClientMiscApi.getBulletins(); | |
| } catch (error) { | |
| logError(error); | |
| throw error; | |
| } | |
| }, |
🤖 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 `@front_end/src/app/`(main)/components/bulletins_client.tsx around lines 48 -
55, The query function for React Query (queryFn) currently catches errors from
ClientMiscApi.getBulletins, logs them via logError, and returns [] which
overwrites valid SSR/initialData; change the catch to log the error with
logError but then rethrow (or throw the caught error) instead of returning an
empty array so React Query can preserve the last successful data (leave
ClientMiscApi.getBulletins and queryFn as the referenced symbols to locate the
code).
Comment on lines
+76
to
+79
| syncedDismissedBulletinIdsRef.current.add(bulletinId); | ||
| void dismissBulletin(bulletinId).catch((error) => { | ||
| logError(error); | ||
| }); |
Contributor
There was a problem hiding this comment.
A failed dismiss sync is treated as permanently synced.
The id is added to syncedDismissedBulletinIdsRef before the request completes and never removed on failure, so one transient error prevents any retry for the rest of the session.
Suggested fix
syncedDismissedBulletinIdsRef.current.add(bulletinId);
void dismissBulletin(bulletinId).catch((error) => {
+ syncedDismissedBulletinIdsRef.current.delete(bulletinId);
logError(error);
});🤖 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 `@front_end/src/app/`(main)/components/bulletins_client.tsx around lines 76 -
79, The code adds bulletinId to syncedDismissedBulletinIdsRef.current before the
network call and never removes it on failure, so transient errors permanently
mark a dismiss as synced; change the logic in the dismiss flow that uses
syncedDismissedBulletinIdsRef and dismissBulletin so the id is only added after
dismissBulletin resolves successfully, or if you must optimistically add it then
remove it inside the catch handler (using
syncedDismissedBulletinIdsRef.current.delete(bulletinId)) and rethrow/log via
logError to allow retries; ensure all references to
syncedDismissedBulletinIdsRef, dismissBulletin, logError, and bulletinId are
updated accordingly.
Comment on lines
+13
to
+19
| return [ | ||
| ...new Set( | ||
| value | ||
| .split(",") | ||
| .map((id) => Number.parseInt(id, 10)) | ||
| .filter(Number.isFinite) | ||
| ), |
Contributor
There was a problem hiding this comment.
Use strict positive-integer validation for bulletin IDs.
parseInt and Number.isFinite currently allow malformed/invalid values (e.g., "12abc", negative, decimals). Since these are bulletin IDs, parsing/serialization should only admit positive integers.
Suggested hardening
export const parseDismissedBulletinIds = (value?: string | null): number[] => {
if (!value) {
return [];
}
return [
...new Set(
value
.split(",")
- .map((id) => Number.parseInt(id, 10))
- .filter(Number.isFinite)
+ .map((id) => id.trim())
+ .filter((id) => /^\d+$/.test(id))
+ .map((id) => Number(id))
+ .filter(Number.isSafeInteger)
+ .filter((id) => id > 0)
),
];
};
export const serializeDismissedBulletinIds = (ids: Iterable<number>) =>
[...new Set(ids)]
- .filter(Number.isFinite)
+ .filter(Number.isSafeInteger)
+ .filter((id) => id > 0)
.slice(-MAX_DISMISSED_BULLETIN_IDS)
.join(",");Also applies to: 23-27
🤖 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 `@front_end/src/app/`(main)/components/bulletins_shared.ts around lines 13 -
19, The current parsing chain (value.split(...).map(id => Number.parseInt(id,
10)).filter(Number.isFinite)) accepts malformed entries like "12abc", negatives
and decimals; replace it with strict positive-integer validation: when mapping
each raw id from value.split(...) validate the raw string with a digits-only
check (e.g. /^\d+$/) or use Number and Number.isInteger plus > 0 before
including it, and then dedupe as before; update both occurrences (the shown
parseInt usage and the same block at lines 23-27) to only accept strictly
positive integers and reject all other inputs.
Comment on lines
+56
to
+58
| <span className="whitespace-nowrap text-[10px] text-gray-700 dark:text-gray-700-dark"> | ||
| translated by | ||
| </span> |
Contributor
There was a problem hiding this comment.
Hardcoded English string should use i18n.
The string "translated by" is hardcoded. Use t() from useTranslations() for consistency with the rest of the component.
🌐 Proposed fix using i18n
- <span className="whitespace-nowrap text-[10px] text-gray-700 dark:text-gray-700-dark">
- translated by
+ <span className="whitespace-nowrap text-[10px] text-gray-700 dark:text-gray-700-dark">
+ {t("translatedBy")}
</span>Based on learnings: Do not hardcode English strings in TSX components. In the Metaculus frontend (Next.js App Router), prefer using useTranslations() and i18n strings for UI text across components.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <span className="whitespace-nowrap text-[10px] text-gray-700 dark:text-gray-700-dark"> | |
| translated by | |
| </span> | |
| <span className="whitespace-nowrap text-[10px] text-gray-700 dark:text-gray-700-dark"> | |
| {t("translatedBy")} | |
| </span> |
🤖 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 `@front_end/src/app/`(main)/components/content_translated_banner/index.tsx
around lines 56 - 58, The span contains a hardcoded English label "translated
by"; replace it with the i18n helper by importing and using useTranslations()
and calling t(...) where this component (e.g., the ContentTranslatedBanner
component or its functional scope) renders that span; update the JSX to use
t("translated_by") (or the appropriate key you add to your translation files) so
the string is localized and ensure the translation key exists in the locale JSON
used by useTranslations().
Comment on lines
84
to
87
| const offset = isLargeScreen | ||
| ? DESKTOP_SCROLL_OFFSET | ||
| : MOBILE_SCROLL_OFFSET; | ||
| : DESKTOP_SCROLL_OFFSET + topChromeHeight + 16; | ||
|
|
Contributor
There was a problem hiding this comment.
Desktop TOC highlighting still ignores extra chrome height.
topChromeHeight only affects the mobile branch, but the TOC now sticks below top-header on every breakpoint. When bulletins increase --top-chrome-height on desktop, the active section will drift because the scroll offset stays fixed at 66px. Use the real chrome height there too, or add the desktop bulletin delta before computing activeId.
🤖 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 `@front_end/src/app/`(main)/notebooks/components/notebook_content_sections.tsx
around lines 84 - 87, The TOC active-section offset ignores dynamic chrome
height on desktop: update the offset calculation used when computing activeId so
it includes topChromeHeight for both branches (i.e., change the isLargeScreen
branch from using DESKTOP_SCROLL_OFFSET alone to DESKTOP_SCROLL_OFFSET +
topChromeHeight or apply a bulletin delta before computing activeId). Locate the
variables and logic around offset, isLargeScreen, DESKTOP_SCROLL_OFFSET,
topChromeHeight and activeId in notebook_content_sections.tsx and ensure the
desktop path uses the real chrome height when computing the scroll offset for
active section highlighting.
Comment on lines
+29
to
+41
| <FlowHeaderActions> | ||
| <Button className="mr-2 hidden sm:block"> | ||
| {t("exitPredictionFlow")} | ||
| </Button> | ||
| <Button | ||
| className="mr-2 border-none bg-transparent text-gray-0 dark:text-gray-0-dark sm:hidden" | ||
| variant="primary" | ||
| > | ||
| <FontAwesomeIcon | ||
| icon={faRightFromBracket} | ||
| className="h-5 w-5" | ||
| /> | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Keep the exit control actionable in the loading header.
Both Buttons render without an href or onClick, so the loading state shows an exit affordance that does nothing. The mobile icon-only button also needs an accessible label when you wire the action back up.
🤖 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
`@front_end/src/app/`(prediction-flow)/tournament/[slug]/prediction-flow/loading.tsx
around lines 29 - 41, The loading header shows exit controls that are
non-functional; update the two Button usages inside FlowHeaderActions (the text
button that renders t("exitPredictionFlow") and the icon-only Button with
FontAwesomeIcon/faRightFromBracket) to call the real exit handler (pass the
existing exit function or navigation callback as an onClick or provide an href)
and ensure the icon-only Button has an accessible label (e.g., aria-label or
aria-labelledby) so screen readers can announce the action; keep
styling/variants the same while wiring these Buttons to the actual exit
behavior.
Comment on lines
47
to
+48
| def __str__(self): | ||
| text = self.text | ||
| if self.post: | ||
| text = (self.post.short_title or self.post.title)[:50] + "... " + text | ||
| elif self.project: | ||
| text = self.project.name[:50] + "... " + text | ||
| return text[:150] + "..." if len(text) > 150 else text | ||
| return self.text[:150] + "..." if len(self.text) > 150 else self.text |
Contributor
There was a problem hiding this comment.
Strip markup before truncating bulletin labels.
If Bulletin.text now contains sanitized HTML, __str__ will leak raw tags into admin list rows and logs. Converting to plain text first keeps the admin readable.
✂️ Suggested change
+from django.utils.html import strip_tags
...
def __str__(self):
- return self.text[:150] + "..." if len(self.text) > 150 else self.text
+ plain_text = strip_tags(self.text).strip()
+ return plain_text[:150] + "..." if len(plain_text) > 150 else plain_text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def __str__(self): | |
| text = self.text | |
| if self.post: | |
| text = (self.post.short_title or self.post.title)[:50] + "... " + text | |
| elif self.project: | |
| text = self.project.name[:50] + "... " + text | |
| return text[:150] + "..." if len(text) > 150 else text | |
| return self.text[:150] + "..." if len(self.text) > 150 else self.text | |
| from django.utils.html import strip_tags | |
| def __str__(self): | |
| plain_text = strip_tags(self.text).strip() | |
| return plain_text[:150] + "..." if len(plain_text) > 150 else plain_text |
🤖 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 `@misc/models.py` around lines 47 - 48, The Bulletin model's __str__ currently
slices raw HTML from self.text which can leak tags; update Bulletin.__str__ to
first convert/sanitize the stored HTML to plain text (e.g., use
django.utils.html.strip_tags or an equivalent HTML-to-text helper) then truncate
that plaintext to 150 characters and append "..." when longer; add the needed
import (strip_tags) and ensure you call the plain-text variable in the return
value instead of self.text.
Comment on lines
+143
to
145
| BulletinViewedBy.objects.get_or_create( | ||
| bulletin=Bulletin.objects.get(pk=pk), user=user | ||
| ) |
Contributor
There was a problem hiding this comment.
Stale or invalid bulletin ids still 500 this endpoint.
Bulletin.objects.get(pk=pk) raises DoesNotExist, so a harmless stale id turns into a server error before get_or_create runs. Please resolve the bulletin with a 404-safe lookup first.
Suggested fix
- BulletinViewedBy.objects.get_or_create(
- bulletin=Bulletin.objects.get(pk=pk), user=user
- )
+ bulletin = get_object_or_404(Bulletin, pk=pk)
+ BulletinViewedBy.objects.get_or_create(bulletin=bulletin, user=user)🤖 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 `@misc/views.py` around lines 143 - 145, The code directly calls
Bulletin.objects.get(pk=pk) inside BulletinViewedBy.objects.get_or_create which
raises Bulletin.DoesNotExist and causes a 500; first resolve the Bulletin using
a 404-safe lookup (e.g., Django's get_object_or_404 or catch
Bulletin.DoesNotExist and raise Http404) into a local variable (e.g., bulletin)
and then call BulletinViewedBy.objects.get_or_create(bulletin=bulletin,
user=user) so stale/invalid ids return a 404 instead of a server error.
Contributor
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
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
1 participant
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.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores