Conversation
Co-authored-by: Copilot <copilot@github.com>
📝 WalkthroughWalkthroughIntroduces a MobX-backed ChangesUser Activity Model & ActivityList wiring
External types, awards, evaluation, questionnaire defaults
Sequence DiagramsequenceDiagram
participant UP as User Page
participant UEM as UserActivityModel
participant AL as ActivityList
participant API as Activity Service
UP->>UEM: instantiate new UserActivityModel(userId, role)
UP->>AL: render ActivityList with store={UEM}
AL->>AL: initialize store = props.store || new ActivityModel()
AL->>UEM: request loadPage(pageIndex, pageSize, filter)
UEM->>API: GET /... (buildURLData user/role paginated)
API-->>UEM: returns { list, count }
UEM-->>AL: provides page data
AL->>AL: render activities
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates profile activity lists to use a dedicated UserActivityModel, aiming to align the user page with role-based activity endpoints while also cleaning up a few UI and lint-related details elsewhere in the app.
Changes:
- Add
UserActivityModeland switch the user profile tabs to load activities through role-specific stores. - Update
ActivityListso callers can inject a custom store instead of always using its internalActivityModel. - Apply small markup/style cleanups on the home page, activity detail page, and Biome config.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pages/user/[id].tsx |
Rewires profile activity tabs to use UserActivityModel instances for enrollee/creator/staff roles. |
pages/index.tsx |
Minor import reordering and list styling cleanup for the homepage stats section. |
pages/activity/[name]/index.tsx |
Cleans up tab markup and keeps award/rank tabs inside the main Tabs container. |
models/User/index.ts |
Formatting-only change in the top-user ranking request. |
models/Activity/index.ts |
Adds UserActivityModel and changes the enrollment submission request method. |
components/Activity/ActivityList.tsx |
Adds optional injected-store support for activity lists. |
biome.json |
Enables a Biome warning for dangerouslySetInnerHTML. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| store = this.props.store || new ActivityModel(); | ||
|
|
| @toggle('uploading') | ||
| async signOne(name: string, form: Enrollment['form'] = []) { | ||
| await this.client.put(`${this.baseURI}/${name}/enrollment`, { form }); | ||
| await this.client.post(`${this.baseURI}/${name}/enrollment`, { form }); |
| <Tab.Pane eventKey="created"> | ||
| <ActivityList type="created" userId={id} /> | ||
| <ActivityList store={new UserActivityModel(id, 'creator')} /> | ||
| </Tab.Pane> | ||
| <Tab.Pane eventKey="admin"> | ||
| <ActivityList type="admin" userId={id} /> | ||
| <ActivityList store={new UserActivityModel(id, 'staff')} /> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
models/User/index.ts (1)
13-13: Add a clarifying comment for the cross-service endpoint.The endpoint
'activity-log/user-rank'is a legitimate cross-service call to the activity-log service (not the user service). Consider adding an inline comment explaining why this endpoint doesn't follow thebaseURIpattern:// User rankings are managed by the activity-log service, not the user service const { body } = await this.client.get<UserRankListChunk>('activity-log/user-rank');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models/User/index.ts` at line 13, Add an inline clarifying comment immediately above the call to this.client.get<UserRankListChunk>('activity-log/user-rank') in models/User/index.ts explaining that 'activity-log/user-rank' is a cross-service endpoint owned by the activity-log service (not the user service) and therefore intentionally does not use the local baseURI pattern; place the comment next to the call that references the UserRankListChunk type so future readers understand why this endpoint differs from other user service calls.biome.json (1)
76-76: ⚡ Quick winPrefer
"error"+ a targetedbiome-ignorecomment over a global"warn"downgrade.Setting
noDangerouslySetInnerHtmlto"warn"globally means future unsafe usages anywhere in the codebase will silently pass CI. The idiomatic approach is to keep the rule at"error"and suppress it only at the one intentional callsite.♻️ Proposed refactor
- "noDangerouslySetInnerHtml": "warn" + "noDangerouslySetInnerHtml": "error"Then at the callsite in
pages/activity/[name]/index.tsx:- <article dangerouslySetInnerHTML={{ __html: detail }} /> + {/* biome-ignore lint/security/noDangerouslySetInnerHtml: detail is admin-authored HTML, sanitized server-side */} + <article dangerouslySetInnerHTML={{ __html: detail ?? '' }} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@biome.json` at line 76, The rule "noDangerouslySetInnerHtml" was globally downgraded to "warn"; revert it to "error" in biome.json and instead add a targeted suppression at the exact intentional callsite (e.g., the usage in pages/activity/[name]/index.tsx) using a biome-ignore comment for that rule; locate the unsafe usage (search for dangerouslySetInnerHTML in pages/activity/[name]/index.tsx) and add a single-line biome-ignore for noDangerouslySetInnerHtml immediately above that line so the rest of the codebase remains enforced as an error.components/Activity/ActivityList.tsx (1)
52-53: ⚡ Quick win
storeis captured once at construction — prop updates after mount are silently ignored
store = this.props.store || new ActivityModel()is a class field initializer: it runs exactly once whenActivityListis mounted. IfUserDetailPage(anobservercomponent) re-renders and passes a freshUserActivityModelinstance,this.storeretains the original value.PureComponent's shallow diff sees a newstoreprop reference and callsrender(), butrender()readsthis.store, notthis.props.store.This is not a bug when callers memoize the store (see the
useMemosuggestion inpages/user/[id].tsx), but a comment or acomponentDidUpdateguard would prevent accidental misuse in the future:♻️ Option A — guard against unintentional prop drift (minimal change)
export default class ActivityList extends PureComponent<ActivityListProps> { store = this.props.store || new ActivityModel(); + + // `store` is initialised once at mount; pass a stable reference (useMemo) from the parent. + // Replacing the store after mount is not supported without remounting (use a `key` prop).♻️ Option B — honour prop changes reactively
+ componentDidUpdate(prevProps: ActivityListProps) { + if (prevProps.store !== this.props.store && this.props.store) { + this.store = this.props.store; + this.forceUpdate(); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Activity/ActivityList.tsx` around lines 52 - 53, ActivityList currently captures store once via the class field "store = this.props.store || new ActivityModel()", so later updates to props.store are ignored; update the component to either (A) guard against prop drift by implementing componentDidUpdate(prevProps) to detect a changed props.store and replace this.store (and optionally warn) or (B) honor prop changes by removing the cached this.store and always read from this.props.store (falling back to creating a new ActivityModel when undefined) in render; reference ActivityList, the store field, ActivityModel, props.store, and componentDidUpdate when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@models/Activity/index.ts`:
- Around line 169-175: The subclass UserActivityModel is inheriting
ActivityModel's restored field (which runs restore(this, 'Activity') during
super()), causing all instances to share the singleton's localStorage key; to
fix, override the restored field in UserActivityModel to opt out of persistence
by adding a class field like restored = false (so after super() runs the
subclass resets it), ensuring UserActivityModel instances do not call
restore(this, 'Activity') and therefore don't overwrite the singleton
cache—update UserActivityModel (constructor/class body) to define restored =
false and leave ActivityModel.restore logic unchanged.
In `@pages/activity/`[name]/index.tsx:
- Around line 280-282: The Tab rendering currently injects unsanitized HTML via
dangerouslySetInnerHTML using the variable detail; sanitize detail before
rendering and guard against undefined. Install and import DOMPurify (or use
server-side sanitize in getServerSideProps), then when rendering in the
component that contains <Tab ...> and <article dangerouslySetInnerHTML={{
__html: detail }} />, replace detail with a sanitized value (e.g.,
sanitizedDetail = DOMPurify.sanitize(detail || '') ), ensuring DOMPurify is only
invoked in the browser (use the existing isServer() guard) or perform
sanitization inside getServerSideProps and pass the safe string down to the
component.
In `@pages/user/`[id].tsx:
- Around line 157-166: UserDetailPage currently constructs three new
UserActivityModel instances inline in JSX (new UserActivityModel(id, 'enrollee')
/ 'creator' / 'staff'), causing recreation on every observer-triggered re-render
(and redundant restore(...) calls) and also being ignored by mounted
ActivityList components; change to memoize or reuse one model instance per (id,
role) — e.g., create/get models from a stable cache or use React.useMemo keyed
on id and role and pass those cached instances into ActivityList instead of
constructing inline; update references around UserDetailPage, UserActivityModel,
and ActivityList (and remove inline new ... in Tab.Pane) and ensure the
PublicUser/User.id type matches the UserActivityModel constructor parameter
userId:number.
---
Nitpick comments:
In `@biome.json`:
- Line 76: The rule "noDangerouslySetInnerHtml" was globally downgraded to
"warn"; revert it to "error" in biome.json and instead add a targeted
suppression at the exact intentional callsite (e.g., the usage in
pages/activity/[name]/index.tsx) using a biome-ignore comment for that rule;
locate the unsafe usage (search for dangerouslySetInnerHTML in
pages/activity/[name]/index.tsx) and add a single-line biome-ignore for
noDangerouslySetInnerHtml immediately above that line so the rest of the
codebase remains enforced as an error.
In `@components/Activity/ActivityList.tsx`:
- Around line 52-53: ActivityList currently captures store once via the class
field "store = this.props.store || new ActivityModel()", so later updates to
props.store are ignored; update the component to either (A) guard against prop
drift by implementing componentDidUpdate(prevProps) to detect a changed
props.store and replace this.store (and optionally warn) or (B) honor prop
changes by removing the cached this.store and always read from this.props.store
(falling back to creating a new ActivityModel when undefined) in render;
reference ActivityList, the store field, ActivityModel, props.store, and
componentDidUpdate when making the change.
In `@models/User/index.ts`:
- Line 13: Add an inline clarifying comment immediately above the call to
this.client.get<UserRankListChunk>('activity-log/user-rank') in
models/User/index.ts explaining that 'activity-log/user-rank' is a cross-service
endpoint owned by the activity-log service (not the user service) and therefore
intentionally does not use the local baseURI pattern; place the comment next to
the call that references the UserRankListChunk type so future readers understand
why this endpoint differs from other user service calls.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e85857b7-d29d-4360-ae02-e3138514c743
📒 Files selected for processing (7)
biome.jsoncomponents/Activity/ActivityList.tsxmodels/Activity/index.tsmodels/User/index.tspages/activity/[name]/index.tsxpages/index.tsxpages/user/[id].tsx
| export class UserActivityModel extends ActivityModel { | ||
| constructor( | ||
| public userId: number, | ||
| public role: 'creator' | 'staff' | 'enrollee', | ||
| ) { | ||
| super(); | ||
| } |
There was a problem hiding this comment.
UserActivityModel inherits restore(this, 'Activity') — all instances share the same localStorage key as the singleton
ActivityModel line 37 defines restored = !isServer() && restore(this, 'Activity') as a class field initializer. Because UserActivityModel calls super(), that initializer runs for every instance, binding each one to the same 'Activity' localStorage key used by the export default new ActivityModel() singleton (line 185).
Consequences:
- When
UserActivityModel.loadPagestores its paginated result,mobx-restful'srestorereaction writes to'Activity', overwriting the singleton's cache. - Any singleton read that later triggers a
restorereload (e.g., navigation back to the main activity list) may re-hydrate with the stale user-filtered list. - Multiple
UserActivityModelinstances (enrollee / creator / staff) overwrite each other on the same key.
Override restored in the subclass to opt out of persistence:
🛡️ Proposed fix
export class UserActivityModel extends ActivityModel {
+ override restored = false as const;
+
constructor(
public userId: number,
public role: 'creator' | 'staff' | 'enrollee',
) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@models/Activity/index.ts` around lines 169 - 175, The subclass
UserActivityModel is inheriting ActivityModel's restored field (which runs
restore(this, 'Activity') during super()), causing all instances to share the
singleton's localStorage key; to fix, override the restored field in
UserActivityModel to opt out of persistence by adding a class field like
restored = false (so after super() runs the subclass resets it), ensuring
UserActivityModel instances do not call restore(this, 'Activity') and therefore
don't overwrite the singleton cache—update UserActivityModel (constructor/class
body) to define restored = false and leave ActivityModel.restore logic
unchanged.
| <Tab className="pt-2" eventKey="detail" title={t('hackathon_detail')}> | ||
| <article dangerouslySetInnerHTML={{ __html: detail }} /> | ||
| </Tab> |
There was a problem hiding this comment.
Sanitize detail HTML before rendering to prevent XSS.
detail is injected verbatim into the DOM via dangerouslySetInnerHTML. If this field is admin- or user-authored (rich-text editor output from the API), any embedded <script>, inline event handlers, or javascript: URIs will execute in the viewer's browser. The static analysis tool (CWE-79) flagged this as an unsanitized injection.
Additionally, detail may be undefined for hackathons that haven't populated this field, which should be guarded against.
The recommended fix is to sanitize on the client with DOMPurify:
🛡️ Proposed fix
Install DOMPurify:
pnpm add dompurify
pnpm add -D `@types/dompurify`
Apply at the callsite:
+ import DOMPurify from 'dompurify';
- <article dangerouslySetInnerHTML={{ __html: detail }} />
+ <article dangerouslySetInnerHTML={{ __html: isServer() ? '' : DOMPurify.sanitize(detail ?? '') }} />Note: DOMPurify requires a DOM environment, so you must guard with isServer() (already imported) or use the SSR build (dompurify/dist/purify.cjs with a JSDOM window shim). Alternatively, sanitize the HTML on the server before returning it from getServerSideProps using sanitize-html or DOMPurify with JSDOM.
🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 280-280: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pages/activity/`[name]/index.tsx around lines 280 - 282, The Tab rendering
currently injects unsanitized HTML via dangerouslySetInnerHTML using the
variable detail; sanitize detail before rendering and guard against undefined.
Install and import DOMPurify (or use server-side sanitize in
getServerSideProps), then when rendering in the component that contains <Tab
...> and <article dangerouslySetInnerHTML={{ __html: detail }} />, replace
detail with a sanitized value (e.g., sanitizedDetail = DOMPurify.sanitize(detail
|| '') ), ensuring DOMPurify is only invoked in the browser (use the existing
isServer() guard) or perform sanitization inside getServerSideProps and pass the
safe string down to the component.
| <Tab.Content> | ||
| <Tab.Pane eventKey="enrolled"> | ||
| <ActivityList type="enrolled" userId={id} /> | ||
| <ActivityList store={new UserActivityModel(id, 'enrollee')} /> | ||
| </Tab.Pane> | ||
| <Tab.Pane eventKey="created"> | ||
| <ActivityList type="created" userId={id} /> | ||
| <ActivityList store={new UserActivityModel(id, 'creator')} /> | ||
| </Tab.Pane> | ||
| <Tab.Pane eventKey="admin"> | ||
| <ActivityList type="admin" userId={id} /> | ||
| <ActivityList store={new UserActivityModel(id, 'staff')} /> | ||
| </Tab.Pane> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the type of User.id from the hop-service package
echo "=== User interface from hop-service ==="
fd -i "index.d.ts" node_modules/@freecodecamp-chengdu/hop-service --exec grep -n "id" {}
echo ""
echo "=== User type in codebase ==="
rg -n --type ts "interface User|type User" -C 5Repository: FreeCodeCamp-Chengdu/HOP
Length of output: 804
🏁 Script executed:
# Find the pages directory and the specific file
fd "user.*\[id\]" pages/
# Also look for pages/user directory
ls -la pages/user/ 2>/dev/null || echo "pages/user not found"
# Check if it's TypeScript or JavaScript
fd "\[id\]" pages/Repository: FreeCodeCamp-Chengdu/HOP
Length of output: 408
🏁 Script executed:
# Find UserActivityModel definition
rg -n "class UserActivityModel|export.*UserActivityModel" --type tsRepository: FreeCodeCamp-Chengdu/HOP
Length of output: 152
🏁 Script executed:
# Find ActivityList component and check if it's PureComponent
rg -n "class ActivityList|const ActivityList|export.*ActivityList" --type ts --type tsxRepository: FreeCodeCamp-Chengdu/HOP
Length of output: 96
🏁 Script executed:
# Check the User type definition in codebase
rg -n "^export.*interface User|^export.*type User" --type tsRepository: FreeCodeCamp-Chengdu/HOP
Length of output: 142
🏁 Script executed:
# Get the full pages/user/[id].tsx file or at least lines around 157-166
fd -e tsx -e ts "user" pages/ | head -10Repository: FreeCodeCamp-Chengdu/HOP
Length of output: 50
🏁 Script executed:
# Read pages/user/[id].tsx to verify the problematic code
cat -n pages/user/[id].tsxRepository: FreeCodeCamp-Chengdu/HOP
Length of output: 7780
🏁 Script executed:
# Find ActivityList definition
rg -n "ActivityList" --type ts -A 3Repository: FreeCodeCamp-Chengdu/HOP
Length of output: 5468
🏁 Script executed:
# Find UserActivityModel definition and constructor
cat -n models/Activity/index.ts | head -200Repository: FreeCodeCamp-Chengdu/HOP
Length of output: 6694
🏁 Script executed:
# Check for PureComponent in ActivityList
rg -n "PureComponent|extends.*Component" models/Activity/ --type tsRepository: FreeCodeCamp-Chengdu/HOP
Length of output: 50
🏁 Script executed:
# Check User import and type
rg -n "import.*User.*from.*hop-service" --type tsRepository: FreeCodeCamp-Chengdu/HOP
Length of output: 904
new UserActivityModel(...) inline in JSX creates new instances on every re-render
UserDetailPage is wrapped with observer(), so it re-renders whenever any MobX observable it reads changes (e.g., sessionStore.user loading after login). Each render creates three new UserActivityModel instances. Because each instance inherits restore(this, 'Activity'), every re-render triggers three redundant localStorage operations.
Additionally, ActivityList (a PureComponent) stores the store prop in a class field at construction time, so any ActivityList that has already mounted will silently ignore replacement instances passed on subsequent renders.
Memoize the models keyed on id:
🛠️ Proposed fix
-import { FC, useContext, useEffect, useState } from 'react';
+import { FC, useMemo, useContext, useEffect, useState } from 'react';
const UserDetailPage: FC<PublicUser> = observer(({ id, name, avatar }) => {
const { t } = useContext(I18nContext);
const { user } = sessionStore;
const [isOwner, setIsOwner] = useState(false);
+
+ const enrolleeStore = useMemo(() => new UserActivityModel(id, 'enrollee'), [id]);
+ const creatorStore = useMemo(() => new UserActivityModel(id, 'creator'), [id]);
+ const staffStore = useMemo(() => new UserActivityModel(id, 'staff'), [id]);
// ...
<Tab.Pane eventKey="enrolled">
- <ActivityList store={new UserActivityModel(id, 'enrollee')} />
+ <ActivityList store={enrolleeStore} />
</Tab.Pane>
<Tab.Pane eventKey="created">
- <ActivityList store={new UserActivityModel(id, 'creator')} />
+ <ActivityList store={creatorStore} />
</Tab.Pane>
<Tab.Pane eventKey="admin">
- <ActivityList store={new UserActivityModel(id, 'staff')} />
+ <ActivityList store={staffStore} />
</Tab.Pane>Also verify that User.id (the type of id in PublicUser) matches the userId: number constructor parameter of UserActivityModel.
📝 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.
| <Tab.Content> | |
| <Tab.Pane eventKey="enrolled"> | |
| <ActivityList type="enrolled" userId={id} /> | |
| <ActivityList store={new UserActivityModel(id, 'enrollee')} /> | |
| </Tab.Pane> | |
| <Tab.Pane eventKey="created"> | |
| <ActivityList type="created" userId={id} /> | |
| <ActivityList store={new UserActivityModel(id, 'creator')} /> | |
| </Tab.Pane> | |
| <Tab.Pane eventKey="admin"> | |
| <ActivityList type="admin" userId={id} /> | |
| <ActivityList store={new UserActivityModel(id, 'staff')} /> | |
| </Tab.Pane> | |
| <Tab.Content> | |
| <Tab.Pane eventKey="enrolled"> | |
| <ActivityList store={enrolleeStore} /> | |
| </Tab.Pane> | |
| <Tab.Pane eventKey="created"> | |
| <ActivityList store={creatorStore} /> | |
| </Tab.Pane> | |
| <Tab.Pane eventKey="admin"> | |
| <ActivityList store={staffStore} /> | |
| </Tab.Pane> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pages/user/`[id].tsx around lines 157 - 166, UserDetailPage currently
constructs three new UserActivityModel instances inline in JSX (new
UserActivityModel(id, 'enrollee') / 'creator' / 'staff'), causing recreation on
every observer-triggered re-render (and redundant restore(...) calls) and also
being ignored by mounted ActivityList components; change to memoize or reuse one
model instance per (id, role) — e.g., create/get models from a stable cache or
use React.useMemo keyed on id and role and pass those cached instances into
ActivityList instead of constructing inline; update references around
UserDetailPage, UserActivityModel, and ActivityList (and remove inline new ...
in Tab.Pane) and ensure the PublicUser/User.id type matches the
UserActivityModel constructor parameter userId:number.
…rvice Agent-Logs-Url: https://github.com/FreeCodeCamp-Chengdu/HOP/sessions/61124e13-9b17-48ad-afd5-df2fa4af5d6d Co-authored-by: TechQuery <19969570+TechQuery@users.noreply.github.com>
…e naming Agent-Logs-Url: https://github.com/FreeCodeCamp-Chengdu/HOP/sessions/61124e13-9b17-48ad-afd5-df2fa4af5d6d Co-authored-by: TechQuery <19969570+TechQuery@users.noreply.github.com>
Done in commit d292915. Here's a summary of the changes made:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pages/activity/[name]/manage/evaluation.tsx (1)
49-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix misleading
numbertype assertion on string form value.In the submit handler,
formToJSON<{ award: number }>(form)is a type assertion that lies to TypeScript. TheformToJSONutility returns raw DOM input values, which are always strings—not coerced to numbers. At runtime,awardwill be a string, creating a type mismatch ifAward['id']is actually typed asnumberin the service.Either coerce explicitly to the expected type or align the type annotation with the actual string:
Suggested fix
- const { awardStore, store } = this, - form = event.currentTarget, - { award: awardId } = formToJSON<{ award: number }>(form); + const { awardStore, store } = this, + form = event.currentTarget, + { award } = formToJSON<{ award: string }>(form), + awardId = Number(award);🤖 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 `@pages/activity/`[name]/manage/evaluation.tsx around lines 49 - 66, The handler handleSubmit currently asserts formToJSON<{ award: number }>(form) but form inputs are strings; change this to parse/coerce the award value before using it: read award as a string from formToJSON (e.g., formToJSON<{ award: string }>(form) or let raw = formToJSON(form)), then convert award to the correct type (use Number(award) or parseInt(award, 10)) and store that result in awardId before calling awardStore.getOne(awardId) and awardStore.assignmentOf(awardId); ensure the converted awardId matches the expected Award['id'] type used by awardStore.getOne and assignmentStore.updateOne.
🤖 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.
Outside diff comments:
In `@pages/activity/`[name]/manage/evaluation.tsx:
- Around line 49-66: The handler handleSubmit currently asserts formToJSON<{
award: number }>(form) but form inputs are strings; change this to parse/coerce
the award value before using it: read award as a string from formToJSON (e.g.,
formToJSON<{ award: string }>(form) or let raw = formToJSON(form)), then convert
award to the correct type (use Number(award) or parseInt(award, 10)) and store
that result in awardId before calling awardStore.getOne(awardId) and
awardStore.assignmentOf(awardId); ensure the converted awardId matches the
expected Award['id'] type used by awardStore.getOne and
assignmentStore.updateOne.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88439a3e-3381-4f4e-92ba-effd33c53426
📒 Files selected for processing (4)
components/Activity/AwardList.tsxcomponents/Team/TeamAwardAssignment.tsxmodels/Activity/Award.tspages/activity/[name]/manage/evaluation.tsx
✅ Files skipped from review due to trivial changes (1)
- components/Team/TeamAwardAssignment.tsx
|
@copilot 将以下后端接口及其对应类型引入： 把原有问卷问题类型替换，再将原有静态数据作为新建问卷时的备选项，在编辑页面新增一个按钮一键填入： HOP/models/Activity/Question.ts Lines 3 to 77 in 742d0dc |
Agent-Logs-Url: https://github.com/FreeCodeCamp-Chengdu/HOP/sessions/0bf1cb54-d765-4360-b910-f34c12b685aa Co-authored-by: TechQuery <19969570+TechQuery@users.noreply.github.com>
… questions Agent-Logs-Url: https://github.com/FreeCodeCamp-Chengdu/HOP/sessions/0bf1cb54-d765-4360-b910-f34c12b685aa Co-authored-by: TechQuery <19969570+TechQuery@users.noreply.github.com>
Done in commit c82a700. Here's what was changed:
|
| team?: Team; | ||
| award: Award; | ||
| } | ||
| export { Award, AwardAssignment, AwardTarget }; |
There was a problem hiding this comment.
- 所有的类型都直接引用，不要重复导出
- 注意这个包是纯类型包，枚举等运行时不可用，要直接写枚举的具体值，并可能需要断言
There was a problem hiding this comment.
Done in commit 0dca77b:
- Removed
export { Award, AwardAssignment, AwardTarget }frommodels/Activity/Award.ts— all consumers now import directly from@freecodecamp-chengdu/hop-service - Same done for
models/Activity/Question.ts - All imports from
hop-servicechanged toimport typesince the package is type-only - Runtime enum value
AwardTarget.Teamreplaced with the concrete string'team'inevaluation.tsx - Runtime enum values
QuestionType.Text/QuestionType.URLreplaced with'text' as QuestionType/'url' as QuestionType
| ) | ||
| return; | ||
|
|
||
| const defaultQuestions = questions(this.observedContext).map((q, i) => ({ |
There was a problem hiding this comment.
Done in commit 0dca77b — renamed q → question and i → index.
…ring literals Agent-Logs-Url: https://github.com/FreeCodeCamp-Chengdu/HOP/sessions/6bedd3a8-f4ff-4d79-a66f-1c8e5de2d9cb Co-authored-by: TechQuery <19969570+TechQuery@users.noreply.github.com>
Done in commit 0dca77b. Both points addressed — re-exports removed and runtime enum values replaced with string literals. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
models/Activity/index.ts (1)
169-183:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
restoredfield inherited fromActivityModelties everyUserActivityModelinstance to the singleton's'Activity'localStorage key.
ActivityModel.restored = !isServer() && restore(this, 'Activity')runs as a class field initializer for every subclass instance viasuper(), so every per-tabUserActivityModel(creator/staff/enrollee) hydrates from and persists into the same key as the default-export singleton, causing them to overwrite each other and the singleton cache.🛡️ Suggested fix
export class UserActivityModel extends ActivityModel { + override restored = false as const; + constructor( public userId: number, public role: 'creator' | 'staff' | 'enrollee', ) {🤖 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 `@models/Activity/index.ts` around lines 169 - 183, ActivityModel currently initializes restored with a fixed key via ActivityModel.restored = !isServer() && restore(this, 'Activity'), causing every UserActivityModel (created via super()) to hydrate/persist to the same 'Activity' localStorage key and overwrite each other; change the persistence key to be instance-specific (e.g., include userId and role) by refactoring ActivityModel to accept a storageKey (or compute it from this.constructor plus identifiers) and replace restore(this, 'Activity') with restore(this, storageKey), then update UserActivityModel to pass a unique key like `Activity:${this.userId}:${this.role}` (or otherwise compute the unique key before calling super) so each instance uses its own localStorage entry.
🧹 Nitpick comments (4)
pages/activity/[name]/manage/questionnaire.tsx (1)
110-113: 💤 Low valueSequential string IDs from index can collide with later user-added items.
String(index + 1)produces ids'1'..'9'. After filling defaults, if the user adds more questions viaaddQuestionnaireItemand another flow assigns ids based on length, or if defaults are filled twice, ids may collide andaddQuestionnaireItemwill short-circuit with the “question_id_repeat” alert. Consider a more unique scheme (e.g.,crypto.randomUUID()or a monotonic counter prefixed likedefault-${i+1}) to avoid surprise collisions.🤖 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 `@pages/activity/`[name]/manage/questionnaire.tsx around lines 110 - 113, defaultQuestions currently assigns sequential string ids via String(index + 1) which can collide with later user-added items and trigger the question_id_repeat short-circuit in addQuestionnaireItem; replace the sequential id generation in the defaultQuestions mapping (the expression that wraps questions(this.observedContext)) with a unique scheme such as crypto.randomUUID() or a monotonic prefixed id like `default-${index+1}` so each default item has a globally-unique id and cannot collide with user-added ids or repeated defaults.models/Activity/index.ts (1)
169-183: 💤 Low value
UserActivityModelonly specializesloadPage; other inherited operations still hit the globalhackathonendpoint.
baseURIremains'hackathon', so any inheritedTableModelmethods (getOne,updateOne,deleteOne,getList, etc.) invoked on aUserActivityModelinstance will operate on the global hackathon resource, not onuser/${userId}/hackathon/${role}. If onlyloadPageis intended to be used here, consider documenting that or narrowing the public surface; otherwise, overridebaseURI(or those methods) so the user-scoped URL is honored consistently.🤖 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 `@models/Activity/index.ts` around lines 169 - 183, UserActivityModel currently only overrides loadPage so inherited TableModel methods (getOne, updateOne, deleteOne, getList, etc.) still use the default baseURI 'hackathon', causing requests to the global resource; either restrict the public surface or override baseURI (or those specific methods) to use the user-scoped path `user/${this.userId}/hackathon/${this.role}` so all operations target the same endpoint—update the class to set a per-instance baseURI or implement wrapper methods for getOne/updateOne/deleteOne/getList that prepend `user/${this.userId}/hackathon/${this.role}` to requests, and keep loadPage as-is.pages/activity/[name]/register.tsx (1)
26-28: 💤 Low valueLocal
activityStoreshadows the imported singleton.Line 12 imports
activityStore(the singleton) andActivityModel, then line 26 declaresconst activityStore = new ActivityModel();insidegetServerSideProps, shadowing the module-level binding. Functionally fine because the SSR-only block uses a fresh instance, but a different name (e.g.,store) avoids the shadowing and makes intent clearer.🤖 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 `@pages/activity/`[name]/register.tsx around lines 26 - 28, The local declaration const activityStore = new ActivityModel() inside getServerSideProps shadows the imported activityStore singleton; rename the local variable (e.g., to store or activityModel) so it doesn't shadow the module-level activityStore and update subsequent references (status, questionnaire calls) to use the new name; ensure you reference ActivityModel when creating the instance and keep getServerSideProps logic unchanged otherwise.components/Activity/QuestionnaireTable.tsx (1)
37-37: 💤 Low valueConsider the cast pattern consistency before removing
as QuestionType.While
type = 'text'may appear to be valid without the cast, note that thisas QuestionTypepattern is consistently used throughout the codebase (seemodels/Activity/Question.tslines 44, 48, 52, 56, 68). SinceQuestionTypeis imported from the external@freecodecamp-chengdu/hop-servicepackage, the cast may be intentional for type safety or narrowing. If removing it elsewhere, ensure consistency across the codebase and that no TypeScript compiler issues arise.🤖 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 `@components/Activity/QuestionnaireTable.tsx` at line 37, The default parameter in the QuestionnaireTable mapping currently uses type = 'text' without the cast, which breaks the existing pattern of casting to QuestionType; restore the cast so the parameter reads type = 'text' as QuestionType to match the rest of the codebase (see QuestionType usage in models/Activity/Question.ts) and avoid TypeScript narrowing/import mismatches; update the mapping callback signature ({ id, title, options, multiple, type = 'text' as QuestionType, required }, index, { length }) to reintroduce the cast so the component and other modules remain consistent.
🤖 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 `@models/Activity/index.ts`:
- Around line 162-166: The signOne method currently discards the POST response
and returns currentEnrollment?.getSessionOne(), which can be undefined when
currentEnrollment is not set (see enrollmentOf usage) and causes a
redundant/racy GET via getSessionOne(); update signOne to capture and return the
POST response body (the newly created enrollment/session data) and also update
the model state (set currentEnrollment or sessionOne on the ActivityModel) from
that response so callers get the authoritative data without an extra GET; ensure
you check the enrollment POST response shape and map it into the model (or
fallback to calling getSessionOne only if the response lacks the expected data).
In `@models/Activity/Question.ts`:
- Around line 42-45: In models/Activity/Question.ts replace the typo'd
translation key t('linkein_or_cv') with t('linkedin_or_cv') and update all
corresponding translation entries (e.g., translation/en-US.ts, zh-CN.ts,
zh-TW.ts) to add/rename the key to "linkedin_or_cv" so the UI and stored
analytics use the corrected identifier; ensure you remove or alias the old
"linkein_or_cv" key to avoid duplicates.
In `@pages/activity/`[name]/manage/questionnaire.tsx:
- Around line 101-116: The confirmation message in fillWithDefaults currently
reuses t('confirm_to_delete_questionnaire') which is misleading; update
fillWithDefaults to call a new translation key (e.g.,
t('confirm_overwrite_with_defaults')) instead of
t('confirm_to_delete_questionnaire'), then add that key with the suggested copy
("Existing questions will be replaced with the default set. Continue?") to your
translation files (e.g., translation/en-US.ts, zh-CN.ts, zh-TW.ts) so the prompt
accurately reflects that existing questions will be overwritten with defaults.
---
Duplicate comments:
In `@models/Activity/index.ts`:
- Around line 169-183: ActivityModel currently initializes restored with a fixed
key via ActivityModel.restored = !isServer() && restore(this, 'Activity'),
causing every UserActivityModel (created via super()) to hydrate/persist to the
same 'Activity' localStorage key and overwrite each other; change the
persistence key to be instance-specific (e.g., include userId and role) by
refactoring ActivityModel to accept a storageKey (or compute it from
this.constructor plus identifiers) and replace restore(this, 'Activity') with
restore(this, storageKey), then update UserActivityModel to pass a unique key
like `Activity:${this.userId}:${this.role}` (or otherwise compute the unique key
before calling super) so each instance uses its own localStorage entry.
---
Nitpick comments:
In `@components/Activity/QuestionnaireTable.tsx`:
- Line 37: The default parameter in the QuestionnaireTable mapping currently
uses type = 'text' without the cast, which breaks the existing pattern of
casting to QuestionType; restore the cast so the parameter reads type = 'text'
as QuestionType to match the rest of the codebase (see QuestionType usage in
models/Activity/Question.ts) and avoid TypeScript narrowing/import mismatches;
update the mapping callback signature ({ id, title, options, multiple, type =
'text' as QuestionType, required }, index, { length }) to reintroduce the cast
so the component and other modules remain consistent.
In `@models/Activity/index.ts`:
- Around line 169-183: UserActivityModel currently only overrides loadPage so
inherited TableModel methods (getOne, updateOne, deleteOne, getList, etc.) still
use the default baseURI 'hackathon', causing requests to the global resource;
either restrict the public surface or override baseURI (or those specific
methods) to use the user-scoped path
`user/${this.userId}/hackathon/${this.role}` so all operations target the same
endpoint—update the class to set a per-instance baseURI or implement wrapper
methods for getOne/updateOne/deleteOne/getList that prepend
`user/${this.userId}/hackathon/${this.role}` to requests, and keep loadPage
as-is.
In `@pages/activity/`[name]/manage/questionnaire.tsx:
- Around line 110-113: defaultQuestions currently assigns sequential string ids
via String(index + 1) which can collide with later user-added items and trigger
the question_id_repeat short-circuit in addQuestionnaireItem; replace the
sequential id generation in the defaultQuestions mapping (the expression that
wraps questions(this.observedContext)) with a unique scheme such as
crypto.randomUUID() or a monotonic prefixed id like `default-${index+1}` so each
default item has a globally-unique id and cannot collide with user-added ids or
repeated defaults.
In `@pages/activity/`[name]/register.tsx:
- Around line 26-28: The local declaration const activityStore = new
ActivityModel() inside getServerSideProps shadows the imported activityStore
singleton; rename the local variable (e.g., to store or activityModel) so it
doesn't shadow the module-level activityStore and update subsequent references
(status, questionnaire calls) to use the new name; ensure you reference
ActivityModel when creating the instance and keep getServerSideProps logic
unchanged otherwise.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5077a851-ffdb-4e1e-9689-9189d3c1d27a
📒 Files selected for processing (16)
components/Activity/AwardList.tsxcomponents/Activity/QuestionnaireCreate.tsxcomponents/Activity/QuestionnairePreview.tsxcomponents/Activity/QuestionnaireTable.tsxcomponents/Team/TeamAwardAssignment.tsxmodels/Activity/Award.tsmodels/Activity/Question.tsmodels/Activity/Team.tsmodels/Activity/index.tspages/activity/[name]/manage/award.tsxpages/activity/[name]/manage/evaluation.tsxpages/activity/[name]/manage/questionnaire.tsxpages/activity/[name]/register.tsxtranslation/en-US.tstranslation/zh-CN.tstranslation/zh-TW.ts
✅ Files skipped from review due to trivial changes (1)
- components/Activity/QuestionnairePreview.tsx
| async signOne(name: string, form: Enrollment['form'] = []) { | ||
| await this.client.put(`${this.baseURI}/${name}/enrollment`, { form }); | ||
| await this.client.post(`${this.baseURI}/${name}/enrollment`, { form }); | ||
|
|
||
| return this.currentEnrollment?.getSessionOne(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all callers of signOne and whether currentEnrollment is guaranteed to be initialized first.
rg -nP -C3 '\bsignOne\s*\('
rg -nP -C3 '\benrollmentOf\s*\('Repository: FreeCodeCamp-Chengdu/HOP
Length of output: 5175
🏁 Script executed:
# Check the register.tsx file to see if enrollmentOf is called before signOne
head -100 pages/activity/[name]/register.tsxRepository: FreeCodeCamp-Chengdu/HOP
Length of output: 2698
🏁 Script executed:
# Check the full context of the index.tsx file where signOne is called
sed -n '60,130p' pages/activity/[name]/index.tsxRepository: FreeCodeCamp-Chengdu/HOP
Length of output: 1936
🏁 Script executed:
# Check the getSessionOne implementation in EnrollmentModel
rg -nP -A5 'getSessionOne\s*\(' models/Repository: FreeCodeCamp-Chengdu/HOP
Length of output: 1242
🏁 Script executed:
# Check the getOne method in Activity model to see the full initialization flow
sed -n '110,140p' models/Activity/index.tsRepository: FreeCodeCamp-Chengdu/HOP
Length of output: 924
🏁 Script executed:
# Check EnrollmentModel constructor and class definition
sed -n '1,50p' models/Activity/Enrollment.tsRepository: FreeCodeCamp-Chengdu/HOP
Length of output: 1622
🏁 Script executed:
# Check if there's any watch or auto-fetch in the model
rg -nP 'makeAutoObservable|makeObservable|reaction|watch' models/Activity/Enrollment.tsRepository: FreeCodeCamp-Chengdu/HOP
Length of output: 50
The POST enrollment response is discarded and may return undefined in some call paths.
signOne discards the POST response and calls currentEnrollment?.getSessionOne() instead. Two issues:
-
Silent failure in register.tsx: The page's server-side
getOne()is called on a localActivityModelinstance, not the globalactivityStore. When the client callssignOne(),currentEnrollmentisundefinedon the global store, so the return silently fails. TheenrollmentOf()method only creates a new model without fetching data. -
Wasteful redundant fetch: The POST response is discarded and a separate GET request is made via
getSessionOne(). This is wasteful and potentially racy depending on backend semantics.
The index.tsx page is safer (calls enrollmentOf() before signOne()), but still depends on getSessionOne() being called elsewhere to populate sessionOne. Confirm the POST enrollment endpoint already returns the new enrollment data, and either return it directly or defer the GET until actually needed.
🤖 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 `@models/Activity/index.ts` around lines 162 - 166, The signOne method
currently discards the POST response and returns
currentEnrollment?.getSessionOne(), which can be undefined when
currentEnrollment is not set (see enrollmentOf usage) and causes a
redundant/racy GET via getSessionOne(); update signOne to capture and return the
POST response body (the newly created enrollment/session data) and also update
the model state (set currentEnrollment or sessionOne on the ActivityModel) from
that response so callers get the authoritative data without an extra GET; ensure
you check the enrollment POST response shape and map it into the model (or
fallback to calling getSessionOne only if the response lacks the expected data).
| { | ||
| title: t('linkein_or_cv'), | ||
| type: 'url', | ||
| type: 'url' as QuestionType, | ||
| }, |
There was a problem hiding this comment.
Typo: linkein_or_cv should be linkedin_or_cv.
The translation key (and presumably the entries in translation/en-US.ts/zh-CN.ts/zh-TW.ts) use linkein instead of linkedin. Easy to fix now before it becomes a load-bearing string in stored data or analytics.
✏️ Proposed fix (apply across translation files too)
- title: t('linkein_or_cv'),
+ title: t('linkedin_or_cv'),📝 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.
| { | |
| title: t('linkein_or_cv'), | |
| type: 'url', | |
| type: 'url' as QuestionType, | |
| }, | |
| { | |
| title: t('linkedin_or_cv'), | |
| type: 'url' as QuestionType, | |
| }, |
🤖 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 `@models/Activity/Question.ts` around lines 42 - 45, In
models/Activity/Question.ts replace the typo'd translation key
t('linkein_or_cv') with t('linkedin_or_cv') and update all corresponding
translation entries (e.g., translation/en-US.ts, zh-CN.ts, zh-TW.ts) to
add/rename the key to "linkedin_or_cv" so the UI and stored analytics use the
corrected identifier; ensure you remove or alias the old "linkein_or_cv" key to
avoid duplicates.
| fillWithDefaults = () => { | ||
| const { t } = this.observedContext; | ||
|
|
||
| if ( | ||
| activityStore.questionnaire.length > 0 && | ||
| !confirm(t('confirm_to_delete_questionnaire')) | ||
| ) | ||
| return; | ||
|
|
||
| const defaultQuestions = questions(this.observedContext).map((question, index) => ({ | ||
| ...question, | ||
| id: String(index + 1), | ||
| })); | ||
|
|
||
| return activityStore.editQuestionnaireStatus(defaultQuestions); | ||
| }; |
There was a problem hiding this comment.
Confirmation message is misleading for the “fill defaults” action.
fillWithDefaults reuses t('confirm_to_delete_questionnaire'), but this flow overwrites the existing questions with defaults rather than deleting the questionnaire. A user who is OK with replacement may cancel because the prompt says “delete”, and vice versa. Add a dedicated translation key (e.g., confirm_overwrite_with_defaults) for clarity.
🌐 Suggested message wording
if (
activityStore.questionnaire.length > 0 &&
- !confirm(t('confirm_to_delete_questionnaire'))
+ !confirm(t('confirm_overwrite_with_defaults'))
)
return;Add the new key to translation/en-US.ts, zh-CN.ts, zh-TW.ts (e.g., “Existing questions will be replaced with the default set. Continue?”).
🤖 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 `@pages/activity/`[name]/manage/questionnaire.tsx around lines 101 - 116, The
confirmation message in fillWithDefaults currently reuses
t('confirm_to_delete_questionnaire') which is misleading; update
fillWithDefaults to call a new translation key (e.g.,
t('confirm_overwrite_with_defaults')) instead of
t('confirm_to_delete_questionnaire'), then add that key with the suggested copy
("Existing questions will be replaced with the default set. Continue?") to your
translation files (e.g., translation/en-US.ts, zh-CN.ts, zh-TW.ts) so the prompt
accurately reflects that existing questions will be overwritten with defaults.
Summary by CodeRabbit
Chores
Style
New Features
Bug Fixes / Improvements