[refactor] add Sign In pages for multiple OAuth platforms#36
[refactor] add Sign In pages for multiple OAuth platforms#36TechQuery wants to merge 18 commits into
Conversation
Co-authored-by: Copilot <copilot@github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a UserActivityModel and optional ActivityList store prop, switches enrollment to POST, migrates many runtime types to type-only imports, adds "Fill with default questions" (UI + translations), introduces CNB/GitHub sign-in pages, rewrites sessionGuard to require JWT, and updates lint rules. ChangesUser Activity Model & ActivityList wiring
External types, awards, evaluation, questionnaire defaults
Sequence DiagramsequenceDiagram
participant Page as User Page
participant UEM as UserActivityModel
participant AL as ActivityList
participant API as Activity Service
Page->>UEM: instantiate UserActivityModel(userId, role)
Page->>AL: render ActivityList with store={UEM}
AL->>AL: initialize store = props.store || new ActivityModel()
AL->>UEM: request loadPage(pageIndex, pageSize, filter)
UEM->>API: GET /user/{userId}/hackathon/{role}?page...
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
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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>
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.
|
@copilot 新增一个路由为 第三方登录回跳后执行原 同时将原 |
…n unauthenticated Agent-Logs-Url: https://github.com/FreeCodeCamp-Chengdu/HOP/sessions/e48ce33e-e81c-433a-a3da-23f86dce5f18 Co-authored-by: TechQuery <19969570+TechQuery@users.noreply.github.com>
Agent-Logs-Url: https://github.com/FreeCodeCamp-Chengdu/HOP/sessions/e48ce33e-e81c-433a-a3da-23f86dce5f18 Co-authored-by: TechQuery <19969570+TechQuery@users.noreply.github.com>
Done in commit ab2b4a6. Here's what was implemented:
|
…rect/notFound check Agent-Logs-Url: https://github.com/FreeCodeCamp-Chengdu/HOP/sessions/d7957cea-ce4a-4e0d-b609-a5c5c7a19422 Co-authored-by: TechQuery <19969570+TechQuery@users.noreply.github.com>
|
@copilot 新建一个路由为 该页面服务端渲染的中间件在收到上述 完成以上功能后，将其加入 |
…on on sign-in page Agent-Logs-Url: https://github.com/FreeCodeCamp-Chengdu/HOP/sessions/001771e3-772f-48e3-a571-fd6349a07e9b Co-authored-by: TechQuery <19969570+TechQuery@users.noreply.github.com>
Done in commit 39f0061. Here's what was implemented:
Translations: added |
… to pages Agent-Logs-Url: https://github.com/FreeCodeCamp-Chengdu/HOP/sessions/d649c696-228a-46c4-a84e-601b7207e709 Co-authored-by: TechQuery <19969570+TechQuery@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
pages/user/OAuth/CNB.tsx (1)
49-51: ⚡ Quick winAdd a timeout to the CNB API fetch.
A slow/hung response from
api.cnb.coolwill block the SSR render until the platform-level timeout fires. UseAbortSignal.timeout(...)to bound the wait.🛡️ Proposed fix
- const response = await fetch(`${CNB_API_BASE}/user`, { - headers: { Authorization: `Bearer ${CNB_token}` }, - }); + const response = await fetch(`${CNB_API_BASE}/user`, { + headers: { Authorization: `Bearer ${CNB_token}` }, + signal: AbortSignal.timeout(5000), + });🤖 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/user/OAuth/CNB.tsx` around lines 49 - 51, The fetch to `${CNB_API_BASE}/user` using `CNB_token` must include an AbortSignal timeout so a slow/hung `api.cnb.cool` response cannot block SSR; update the fetch call to pass a `signal: AbortSignal.timeout(<ms>)` in the options (choose an appropriate timeout, e.g. 3000–5000ms), and wrap the request in try/catch to handle an aborted request (check for DOMException/AbortError) so you can return/handle a timeout error path instead of letting the render hang; modify the code around the existing fetch (the block referencing CNB_API_BASE and CNB_token) to add the signal and error handling.
🤖 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 `@pages/api/core.ts`:
- Line 86: The console.error call currently logs the raw JWT (the JWT variable)
alongside the JsonWebTokenError message; remove the JWT from logs (or replace it
with a short non-sensitive fingerprint like a truncated hash) so sensitive token
payloads are not written to logs—locate the console.error((error as
JsonWebTokenError).message, JWT) usage and change it to log only the error
message or a hash/fingerprint of JWT instead.
- Around line 77-78: The cookie name is inconsistent: sessionGuard reads
req.cookies.JWT while parseJWT and verifyJWT expect cookie: 'token'; fix by
making the cookie name consistent across code — either change where the cookie
is set (in signIn.tsx) to use 'token' or update parseJWT and verifyJWT to use
cookie: 'JWT'. Locate the functions parseJWT and verifyJWT and adjust their
cookie option to 'JWT' (or update the setter in signIn.tsx to set 'token') so
all reads/writes use the same cookie name.
In `@pages/user/OAuth/CNB.tsx`:
- Around line 25-26: The callback query value (callback, destination) is
user-controlled and currently used directly in redirects; validate and sanitize
it before use by ensuring it is a relative same-origin path: parse the callback,
reject or strip any scheme/host, and only accept values that start with '/' and
do not contain full origin or "://" (otherwise fall back to '/'); update where
destination is computed (the const destination = callback + '' and any use in
redirect.destination) to use this sanitized path so external URLs cannot be used
for open redirects.
- Around line 30-35: The Set-Cookie header for CNB_token is missing the HttpOnly
attribute, exposing the server-only CNB_token to client-side scripts; update the
cookie construction in the Set-Cookie call that sets `CNB_token=${token}` (in
pages/user/OAuth/CNB.tsx) to include `HttpOnly` alongside Path, Secure, and
SameSite so the cookie cannot be accessed via document.cookie (keep the existing
filter(Boolean).join('; ') pattern and add 'HttpOnly' to the attributes array).
- Around line 80-97: The form currently uses the default GET (Form in CNB.tsx)
which leaks the personal access token via the URL; change the <Form> to submit
via POST (add method="post") and ensure the input with name="token" is read from
the request body in getServerSideProps instead of from query; update server-side
handling to parse POST body (or accept the cookie built client-side) and set the
auth cookie from req.body.token, keeping existing symbols Form, name="token",
and getServerSideProps as the touchpoints to locate and fix the code.
- Around line 78-79: The translation call t('sign_in_with', 'CNB') is using a
string instead of the options object required for function-valued templates;
update each usage (the PageHead title prop and the h1 in the CNB component and
the two occurrences in the signIn component) to call t('sign_in_with', {
platform: 'CNB' }) (or { platform: variableName } if using a variable) so the
function-style translation is invoked with the platform parameter.
In `@pages/user/OAuth/GitHub.tsx`:
- Around line 10-13: The redirect currently uses the unvalidated query variable
`callback` (assigned to `destination`) which allows open redirects; update the
logic around `callback`/`destination` to only allow same-origin relative paths
by validating that `callback` is a safe path (e.g., non-empty string starting
with a single '/' and not starting with '//' or containing a scheme like
'http:'), and fall back to '/' if invalid. Locate the code that reads `const {
callback = '/' } = query; const destination = callback + ''; return { redirect:
{ destination, permanent: false } };` and replace it with validation that
rejects or normalizes external URLs before returning the redirect. Ensure you
check for leading '//' and URI schemes and only permit paths like '/some/path'
(no host or protocol).
---
Nitpick comments:
In `@pages/user/OAuth/CNB.tsx`:
- Around line 49-51: The fetch to `${CNB_API_BASE}/user` using `CNB_token` must
include an AbortSignal timeout so a slow/hung `api.cnb.cool` response cannot
block SSR; update the fetch call to pass a `signal: AbortSignal.timeout(<ms>)`
in the options (choose an appropriate timeout, e.g. 3000–5000ms), and wrap the
request in try/catch to handle an aborted request (check for
DOMException/AbortError) so you can return/handle a timeout error path instead
of letting the render hang; modify the code around the existing fetch (the block
referencing CNB_API_BASE and CNB_token) to add the signal and error handling.
🪄 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: 91f24cfe-0230-4c03-bd21-5cff378ff7bb
📒 Files selected for processing (15)
biome.jsoncomponents/Activity/QuestionnaireCreate.tsxcomponents/Team/TeamAwardAssignment.tsxcomponents/User/UserBar.tsxmodels/Activity/index.tsmodels/User/Session.tspages/activity/[name]/manage/evaluation.tsxpages/api/core.tspages/user/OAuth/CNB.tsxpages/user/OAuth/GitHub.tsxpages/user/me.tsxpages/user/signIn.tsxtranslation/en-US.tstranslation/zh-CN.tstranslation/zh-TW.ts
💤 Files with no reviewable changes (1)
- pages/user/me.tsx
✅ Files skipped from review due to trivial changes (5)
- components/User/UserBar.tsx
- pages/user/signIn.tsx
- biome.json
- translation/en-US.ts
- translation/zh-TW.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- components/Team/TeamAwardAssignment.tsx
- models/Activity/index.ts
- pages/activity/[name]/manage/evaluation.tsx
| export const sessionGuard = compose<DataObject, JWTProps<User>>(async ({ req }, next) => { | ||
| const { JWT = '' } = req.cookies; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every place a JWT-style cookie is set or read so we can confirm naming consistency.
rg -nP -C2 "Set-Cookie|cookies?\.(JWT|token)\b|res\.cookie\s*\(|setHeader\(\s*['\"]Set-Cookie" --type=ts --type=tsx
rg -nP -C2 "cookie\s*:\s*['\"](JWT|token)['\"]" --type=ts --type=tsxRepository: FreeCodeCamp-Chengdu/HOP
Length of output: 124
🏁 Script executed:
#!/bin/bash
# Find JWT/token cookie references in TypeScript files
rg -n "cookies?\.(JWT|token)\b" --type=ts
rg -n "cookie\s*:\s*['\"]?(JWT|token)['\"]?" --type=ts
rg -n "Set-Cookie|setHeader.*Set-Cookie" --type=ts
rg -n "res\.cookie" --type=tsRepository: FreeCodeCamp-Chengdu/HOP
Length of output: 344
🏁 Script executed:
#!/bin/bash
# Check the specific file mentioned in the review
head -30 pages/api/core.ts
tail -n +70 pages/api/core.ts | head -20Repository: FreeCodeCamp-Chengdu/HOP
Length of output: 1688
🏁 Script executed:
#!/bin/bash
# Check the Set-Cookie calls in signIn.tsx
sed -n '35,45p' pages/user/signIn.tsx
# Check the Set-Cookie calls in CNB.tsx
sed -n '25,35p' pages/user/OAuth/CNB.tsx
sed -n '58,68p' pages/user/OAuth/CNB.tsxRepository: FreeCodeCamp-Chengdu/HOP
Length of output: 1175
🏁 Script executed:
#!/bin/bash
# Search for all places where cookies are being set with values
rg -n "Set-Cookie.*token|Set-Cookie.*JWT" --type=ts -B2 -A2
# Also search for cookie setting patterns with more context
rg -n "res\.cookie.*token|res\.cookie.*JWT" --type=ts -B2 -A2Repository: FreeCodeCamp-Chengdu/HOP
Length of output: 50
Fix cookie name inconsistency: JWT vs token.
The server sets a cookie named JWT (signIn.tsx), but parseJWT and verifyJWT are configured to read from a cookie named token (lines 17, 21). This mismatch means parseJWT/verifyJWT will not find the JWT. While sessionGuard correctly reads from req.cookies.JWT, other parts of the code using parseJWT/verifyJWT will silently fail to locate the token. Either rename the cookie to token where it's set, or update parseJWT/verifyJWT to use cookie: 'JWT'.
🤖 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/api/core.ts` around lines 77 - 78, The cookie name is inconsistent:
sessionGuard reads req.cookies.JWT while parseJWT and verifyJWT expect cookie:
'token'; fix by making the cookie name consistent across code — either change
where the cookie is set (in signIn.tsx) to use 'token' or update parseJWT and
verifyJWT to use cookie: 'JWT'. Locate the functions parseJWT and verifyJWT and
adjust their cookie option to 'JWT' (or update the setter in signIn.tsx to set
'token') so all reads/writes use the same cookie name.
|
|
||
| return 'props' in nextResult ? { props: { ...nextResult.props, jwtPayload } } : nextResult; | ||
| } catch (error) { | ||
| console.error((error as JsonWebTokenError).message, JWT); |
There was a problem hiding this comment.
Don't log the raw JWT cookie value.
console.error((error as JsonWebTokenError).message, JWT) writes the user's bearer token to logs. Even when verification fails, the token's payload is still decodable and frequently contains PII (email, user id). Log forwarders, error trackers, and CI logs will then carry sensitive data. If signature verification ever fails for a transient reason (e.g. clock skew, secret rotation), a valid token could end up in logs.
Drop the token from the log line, or at most log a short fingerprint.
🛡️ Proposed fix
- console.error((error as JsonWebTokenError).message, JWT);
+ console.error('JWT verification failed:', (error as JsonWebTokenError).message);🤖 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/api/core.ts` at line 86, The console.error call currently logs the raw
JWT (the JWT variable) alongside the JsonWebTokenError message; remove the JWT
from logs (or replace it with a short non-sensitive fingerprint like a truncated
hash) so sensitive token payloads are not written to logs—locate the
console.error((error as JsonWebTokenError).message, JWT) usage and change it to
log only the error message or a hash/fingerprint of JWT instead.
| res.setHeader( | ||
| 'Set-Cookie', | ||
| [`CNB_token=${token}`, 'Path=/', isProduction ? 'Secure' : '', 'SameSite=Lax'] | ||
| .filter(Boolean) | ||
| .join('; '), | ||
| ); |
There was a problem hiding this comment.
Token cookie must be HttpOnly.
CNB_token is only read on the server (line 44, 50). Without HttpOnly, any XSS on the site can read and exfiltrate the user's CNB personal access token via document.cookie. Add HttpOnly to the attributes.
🛡️ Proposed fix
res.setHeader(
'Set-Cookie',
- [`CNB_token=${token}`, 'Path=/', isProduction ? 'Secure' : '', 'SameSite=Lax']
+ [
+ `CNB_token=${encodeURIComponent(token + '')}`,
+ 'Path=/',
+ 'HttpOnly',
+ isProduction ? 'Secure' : '',
+ 'SameSite=Lax',
+ ]
.filter(Boolean)
.join('; '),
);📝 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.
| res.setHeader( | |
| 'Set-Cookie', | |
| [`CNB_token=${token}`, 'Path=/', isProduction ? 'Secure' : '', 'SameSite=Lax'] | |
| .filter(Boolean) | |
| .join('; '), | |
| ); | |
| res.setHeader( | |
| 'Set-Cookie', | |
| [ | |
| `CNB_token=${encodeURIComponent(token + '')}`, | |
| 'Path=/', | |
| 'HttpOnly', | |
| isProduction ? 'Secure' : '', | |
| 'SameSite=Lax', | |
| ] | |
| .filter(Boolean) | |
| .join('; '), | |
| ); |
🤖 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/user/OAuth/CNB.tsx` around lines 30 - 35, The Set-Cookie header for
CNB_token is missing the HttpOnly attribute, exposing the server-only CNB_token
to client-side scripts; update the cookie construction in the Set-Cookie call
that sets `CNB_token=${token}` (in pages/user/OAuth/CNB.tsx) to include
`HttpOnly` alongside Path, Secure, and SameSite so the cookie cannot be accessed
via document.cookie (keep the existing filter(Boolean).join('; ') pattern and
add 'HttpOnly' to the attributes array).
TechQuery
changed the title
|
@copilot 修复以下几条意见及全篇其它地方的同类问题： |
Agent-Logs-Url: https://github.com/FreeCodeCamp-Chengdu/HOP/sessions/50e87b4b-5ff6-46f1-a4a8-f3b5f3f67cd8 Co-authored-by: TechQuery <19969570+TechQuery@users.noreply.github.com>
Done in commit e3d1590:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
pages/user/OAuth/CNB.tsx (3)
55-60:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win[DUPLICATE] Token cookie must be
HttpOnly.This issue was flagged in a previous review and remains unfixed. The
CNB_tokencookie is only read server-side (lines 72, 77-78) but is accessible to client JavaScript. Any XSS vulnerability on the site can exfiltrate the user's CNB personal access token.🛡️ Proposed fix
res.setHeader( 'Set-Cookie', - [`CNB_token=${token}`, 'Path=/', isProduction ? 'Secure' : '', 'SameSite=Lax'] + [ + `CNB_token=${encodeURIComponent(token)}`, + 'Path=/', + 'HttpOnly', + isProduction ? 'Secure' : '', + 'SameSite=Lax', + ] .filter(Boolean) .join('; '), );🤖 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/user/OAuth/CNB.tsx` around lines 55 - 60, The CNB_token cookie is missing the HttpOnly flag making it accessible to client JS; update the cookie set in the res.setHeader call in CNB.tsx (the code building 'CNB_token=${token}') to include HttpOnly (in addition to existing Path, Secure, SameSite flags) so the token is only sent by the browser and not exposed to JavaScript.
106-107:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win[DUPLICATE] Fix translation function call syntax for parameterized templates.
This issue was flagged in a previous review and remains unfixed. The
sign_in_withtranslation is a function-valued template that expects an options object, but you're passing a plain string. This will cause incorrect rendering or a runtime error.🔧 Proposed fix
- <PageHead title={t('sign_in_with', 'CNB')} /> - <h1>{t('sign_in_with', 'CNB')}</h1> + <PageHead title={t('sign_in_with', { platform: 'CNB' })} /> + <h1>{t('sign_in_with', { platform: 'CNB' })}</h1>As noted in the previous review, this same fix applies to
pages/user/signIn.tsx(lines 85, 95).🤖 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/user/OAuth/CNB.tsx` around lines 106 - 107, The translation key sign_in_with is a parameterized (function-valued) template but the code calls t('sign_in_with', 'CNB') as if passing a plain string; update the two occurrences (the PageHead title and the h1 rendering where t('sign_in_with', 'CNB') is used) to pass an options object instead, e.g. t('sign_in_with', { provider: 'CNB' }), so the template receives the expected parameters and renders correctly; apply the same change for the other identical t('sign_in_with', 'CNB') usages in the codebase.
57-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winURL-encode the token value in the cookie.
If the CNB token contains special characters like
=,;, or spaces, the cookie will be malformed and parsing will fail. UseencodeURIComponent(token)to safely encode the value.🔧 Proposed fix
- [`CNB_token=${token}`, 'Path=/', isProduction ? 'Secure' : '', 'SameSite=Lax'] + [`CNB_token=${encodeURIComponent(token)}`, 'Path=/', isProduction ? 'Secure' : '', 'SameSite=Lax']Also remember to decode when reading:
decodeURIComponent(req.cookies.CNB_token || '')at line 72.🤖 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/user/OAuth/CNB.tsx` at line 57, The CNB cookie value should be URL-encoded to avoid breaking the cookie when token contains special chars: update the code that builds the cookie string (the array entry using `CNB_token=${token}`) to use `encodeURIComponent(token)` instead, and ensure the corresponding read path decodes the cookie (use `decodeURIComponent(req.cookies.CNB_token || '')`) where the token is read.
🧹 Nitpick comments (1)
pages/user/OAuth/CNB.tsx (1)
41-99: Consider rate limiting for CNB token validation.The current implementation allows unlimited form submissions and CNB API calls per user/IP. This could enable brute-force attacks on CNB tokens or abuse of the CNB API quota.
Adding rate limiting (e.g., using a Redis-backed middleware or Next.js middleware with a memory store) would reduce the risk of abuse. For example:
- Limit token submission attempts to 5 per IP per minute
- Limit CNB API validation calls to prevent quota exhaustion
This is an infrastructure concern rather than a code fix, but worth considering before production deployment.
🤖 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/user/OAuth/CNB.tsx` around lines 41 - 99, getServerSideProps allows unlimited POST token submissions and CNB API calls; add a rate-limit check (e.g., Redis or in-memory limiter) at the start of the POST branch (before parseFormBody) to throttle token submissions (suggest 5 attempts per IP per minute) and another check before calling fetch(${CNB_API_BASE}/user) when CNB_token is present to protect API quota. If a limit is exceeded, immediately return a 429 response or props with a clear error (include Retry-After header), and log the event; ensure the limiter keys are derived from req.socket.remoteAddress or similar and reference getServerSideProps, CNB_token, parseFormBody, and CNB_API_BASE when implementing.
🤖 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 `@pages/user/OAuth/CNB.tsx`:
- Around line 29-35: The data handler continues appending chunks after calling
req.destroy(), so add an early-exit flag (e.g., let aborted = false) in the
scope above the req.on('data', ...) handler; inside that handler check if
(aborted) return; when size exceeds MAX_BODY_BYTES set aborted = true before
calling req.destroy(new Error('Request body too large')) to prevent further
processing and appending to data; update any related logic that relies on
size/data accordingly (references: the req.on('data', ...) callback, size,
MAX_BODY_BYTES, and req.destroy).
---
Duplicate comments:
In `@pages/user/OAuth/CNB.tsx`:
- Around line 55-60: The CNB_token cookie is missing the HttpOnly flag making it
accessible to client JS; update the cookie set in the res.setHeader call in
CNB.tsx (the code building 'CNB_token=${token}') to include HttpOnly (in
addition to existing Path, Secure, SameSite flags) so the token is only sent by
the browser and not exposed to JavaScript.
- Around line 106-107: The translation key sign_in_with is a parameterized
(function-valued) template but the code calls t('sign_in_with', 'CNB') as if
passing a plain string; update the two occurrences (the PageHead title and the
h1 rendering where t('sign_in_with', 'CNB') is used) to pass an options object
instead, e.g. t('sign_in_with', { provider: 'CNB' }), so the template receives
the expected parameters and renders correctly; apply the same change for the
other identical t('sign_in_with', 'CNB') usages in the codebase.
- Line 57: The CNB cookie value should be URL-encoded to avoid breaking the
cookie when token contains special chars: update the code that builds the cookie
string (the array entry using `CNB_token=${token}`) to use
`encodeURIComponent(token)` instead, and ensure the corresponding read path
decodes the cookie (use `decodeURIComponent(req.cookies.CNB_token || '')`) where
the token is read.
---
Nitpick comments:
In `@pages/user/OAuth/CNB.tsx`:
- Around line 41-99: getServerSideProps allows unlimited POST token submissions
and CNB API calls; add a rate-limit check (e.g., Redis or in-memory limiter) at
the start of the POST branch (before parseFormBody) to throttle token
submissions (suggest 5 attempts per IP per minute) and another check before
calling fetch(${CNB_API_BASE}/user) when CNB_token is present to protect API
quota. If a limit is exceeded, immediately return a 429 response or props with a
clear error (include Retry-After header), and log the event; ensure the limiter
keys are derived from req.socket.remoteAddress or similar and reference
getServerSideProps, CNB_token, parseFormBody, and CNB_API_BASE when
implementing.
🪄 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: b7d56344-13d0-4a6f-8ed0-2a227db3805f
📒 Files selected for processing (4)
pages/user/OAuth/CNB.tsxpages/user/OAuth/GitHub.tsxpages/user/signIn.tsxutils/url.ts
✅ Files skipped from review due to trivial changes (1)
- utils/url.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- pages/user/OAuth/GitHub.tsx
- pages/user/signIn.tsx
| req.on('data', (chunk: Buffer) => { | ||
| size += chunk.length; | ||
| if (size > MAX_BODY_BYTES) { | ||
| req.destroy(new Error('Request body too large')); | ||
| return; | ||
| } | ||
| data += chunk.toString(); |
There was a problem hiding this comment.
Add early-exit flag to prevent processing chunks after body size limit.
After calling req.destroy(), subsequent data events may still fire before the error event propagates. The handler continues appending chunks to data, which is unnecessary and could delay the rejection.
🛡️ Proposed fix
const parseFormBody = (req: IncomingMessage) =>
new Promise<URLSearchParams>((resolve, reject) => {
let data = '';
let size = 0;
+ let destroyed = false;
req.on('data', (chunk: Buffer) => {
+ if (destroyed) return;
size += chunk.length;
if (size > MAX_BODY_BYTES) {
+ destroyed = true;
req.destroy(new Error('Request body too large'));
return;
}
data += chunk.toString();
});🤖 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/user/OAuth/CNB.tsx` around lines 29 - 35, The data handler continues
appending chunks after calling req.destroy(), so add an early-exit flag (e.g.,
let aborted = false) in the scope above the req.on('data', ...) handler; inside
that handler check if (aborted) return; when size exceeds MAX_BODY_BYTES set
aborted = true before calling req.destroy(new Error('Request body too large'))
to prevent further processing and appending to data; update any related logic
that relies on size/data accordingly (references: the req.on('data', ...)
callback, size, MAX_BODY_BYTES, and req.destroy).
…packages [fix] some detail bugs
|
@copilot 用 Playwright 阅读 https://api.cnb.cool/ ，给以下方法多加一个可选参数： 用于调用一个新方法，其基本逻辑如下：
git clone GitHub仓库地址 ./
git commit -m "HOP initializes this repository from GitHub仓库地址"
git push --no-verify
|
Agent-Logs-Url: https://github.com/FreeCodeCamp-Chengdu/HOP/sessions/d9f01f59-f078-4953-ad3e-4b614d0820ae Co-authored-by: TechQuery <19969570+TechQuery@users.noreply.github.com>
Done in commit d3508e0.
|
Summary by CodeRabbit
Chores
Style
New Features
Bug Fixes / Improvements