Open
Conversation
📝 WalkthroughWalkthroughAdded three IntEnum types and a sign-in workflow (get_activity_list, pre_sign, sign_in_normal); refined rate limiter and various API helpers; extended video progress tracking with an Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Chaoxing Client
participant API as Client API Layer
participant Activity as Activity Endpoint
User->>Client: Trigger sign-in for course
Client->>API: get_activity_list(course)
API->>Activity: Fetch activities
Activity-->>API: Return list
API-->>Client: Activities returned
Client->>API: pre_sign(course, activity_id)
API->>Activity: Pre-sign request
Activity-->>API: Pre-sign response
API-->>Client: Raw pre-sign text
Client->>API: sign_in_normal(course, activity_id, params)
API->>Activity: Submit sign-in (type/location)
Activity-->>API: Sign-in result
API-->>Client: Result -> User
sequenceDiagram
participant Job as Video Job
participant Tracker as Video Tracker
participant API as Progress API
participant Server as Chaoxing Server
Job->>Tracker: study_video() start
Tracker->>API: video_progress_log(_isdrag=4)
API->>Server: Submit progress with drag flag
Server-->>API: Acknowledge
API-->>Tracker: Progress OK
Tracker->>Server: _refresh_video_status() check
Server-->>Tracker: Status update
Tracker-->>Job: Return StudyResult
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
api/base.py (2)
265-285: Minor:pre_signdoes not return an error indicator on failure.The method logs an error when
status_code != 200but still returnsresp_txtwithout raising or returning a distinct error value. This could mask failures from callers.💡 Consider returning None or raising on failure
resp = s.get('https://mobilelearn.chaoxing.com/newsign/preSign', params=params) resp_txt = resp.text logger.debug("Request url" + resp.url) if resp.status_code != 200: logger.error("Failed to get sign in, return code: " + str(resp.status_code) + "message: " + resp_txt) + return None return resp_txt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/base.py` around lines 265 - 285, pre_sign currently logs errors when resp.status_code != 200 but still returns resp.text, which hides failures from callers; change pre_sign (which uses SessionManager.get_session() and calls s.get(...)) to raise a descriptive exception (including resp.status_code, resp.text and resp.url) on non-200 responses instead of returning resp_txt, or alternatively return None on failure and update callers to handle None — pick one consistent approach and implement it inside pre_sign so callers can detect and handle errors.
200-200: Consider documenting the magic number1024fallback.Returning
1024as a default whenfidis missing from cookies works, but the meaning of this value is unclear. A brief comment would improve maintainability.📝 Add documentation
def get_fid(self): _session = SessionManager.get_session() + # 1024 is a default fid value that works for most API calls when the actual fid is unavailable return _session.cookies.get("fid", 1024)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/base.py` at line 200, The fallback value 1024 used in the expression _session.cookies.get("fid", 1024) is a magic number and needs clarification—add a short inline comment explaining why 1024 is the default (e.g., legacy ID, sentinel, or max/default value) or replace it with a well-named constant (e.g., DEFAULT_FID = 1024) and reference that constant here so the intent of the fallback for the "fid" cookie is clear and maintainable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/base.py`:
- Around line 89-94: The code double-adds extra_wait into the delay: extra_wait
is included in call_wait and then added again when setting self.last_call,
inflating rate-limit delays; fix by setting self.last_call = now + call_wait
(i.e., remove the additional + extra_wait) so last_call and
time.sleep(call_wait) use the same computed delay (referencing variables
base_wait, extra_wait, call_wait and attribute self.last_call in the
rate-limiting logic).
- Around line 510-514: The logger.debug call in api/base.py mixes Python "%"
formatting with Loguru style placeholders; change the call that currently uses
"刷新视频状态返回码异常: {}" % resp.status_code to use Loguru's brace-style placeholder and
pass resp.status_code as an argument (e.g., logger.debug("刷新视频状态返回码异常: {}",
resp.status_code)), leaving the subsequent logger.debug(resp.text) as-is.
- Around line 314-316: The code is logging the Match object `msg` instead of the
captured distance string; update the logic around `pattern`, `resp_txt`, and
`msg` to check that `msg` is not None, extract the captured group via
`msg.group(1)` (or handle missing group safely), and log that extracted value
(e.g. `distance = msg.group(1)` then `logger.warning(f"距离签到位置 {distance}m")`);
ensure you handle the None case (skip logging or log a sensible fallback) so you
never try to call group on None.
In `@main.py`:
- Around line 87-88: The --auto-sign flag is parsed as args.auto_sign but never
used; either remove the flag or wire it into the runtime config and sign-in
workflow. Update build_config_from_args() to accept and set an auto_sign boolean
from args.auto_sign (so the returned config includes config.auto_sign), then in
the main execution path where the config is consumed, branch on config.auto_sign
to invoke the sign-in workflow: call the API methods in api/base.py
(get_activity_list, pre_sign, sign_in_normal) in the appropriate sequence and
handle/report errors; alternatively, if you intend to postpone the feature,
remove the parser.add_argument("--auto-sign", ...) line to avoid dead flags.
---
Nitpick comments:
In `@api/base.py`:
- Around line 265-285: pre_sign currently logs errors when resp.status_code !=
200 but still returns resp.text, which hides failures from callers; change
pre_sign (which uses SessionManager.get_session() and calls s.get(...)) to raise
a descriptive exception (including resp.status_code, resp.text and resp.url) on
non-200 responses instead of returning resp_txt, or alternatively return None on
failure and update callers to handle None — pick one consistent approach and
implement it inside pre_sign so callers can detect and handle errors.
- Line 200: The fallback value 1024 used in the expression
_session.cookies.get("fid", 1024) is a magic number and needs clarification—add
a short inline comment explaining why 1024 is the default (e.g., legacy ID,
sentinel, or max/default value) or replace it with a well-named constant (e.g.,
DEFAULT_FID = 1024) and reference that constant here so the intent of the
fallback for the "fid" cookie is clear and maintainable.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/base.py (2)
602-607:⚠️ Potential issue | 🟠 MajorApply the refreshed metadata back to the loop state.
The recovery branch fetches a new
durationandplayTime, but it only storesdurationin a throwaway_durationvariable. After a 403, the next iteration can keep using the stale duration, andplay_timemay become a string straight from the status payload.🐛 Proposed fix
- _dtoken = refreshed_meta.get("dtoken", _dtoken) - _duration = refreshed_meta.get("duration", duration) - play_time = refreshed_meta.get("playTime", play_time) - - logger.debug("Refreshed token: {}, duration: {}, play time: {}", _dtoken, _duration, play_time) + _dtoken = refreshed_meta.get("dtoken", _dtoken) + duration = int(refreshed_meta.get("duration", duration)) + play_time = int(refreshed_meta.get("playTime", play_time)) + + logger.debug("Refreshed token: {}, duration: {}, play time: {}", _dtoken, duration, play_time) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/base.py` around lines 602 - 607, The refreshed_meta handling currently assigns to throwaway locals and leaves loop state stale; in the recovery branch update the actual loop variables by assigning _dtoken = refreshed_meta["dtoken"], duration = refreshed_meta["duration"], and play_time = int(refreshed_meta["playTime"]) (or coerce/validate numeric) instead of using .get fallbacks, and if any of those keys are missing raise/log an error rather than silently falling back; update the values before the logger.debug call so the next iteration uses the refreshed state.
391-417:⚠️ Potential issue | 🟠 MajorThe new fast-path still cannot send
rt=4.Line 577 now passes
_isdrag=4, butparams["rt"]is still derived from_job['rt']or the fallback{0.9, 1}. This path changesisdrag, notrt, so the advertised seekable-video behavior is still missing.🐛 One minimal fix
- rt = _job['rt'] + rt = 4 if _isdrag == 4 else _job['rt']Also applies to: 577-578
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/base.py` around lines 391 - 417, The fast-path sets _isdrag but never updates params["rt"], so when callers pass _isdrag=4 the request still uses _job["rt"] or the old fallback; update the params construction (the params dict creation where "isdrag" and "view" are set) to explicitly set params["rt"] based on the _isdrag argument (e.g., override params["rt"] = _isdrag when _isdrag is provided or when it indicates the seekable/fast-path value) instead of relying solely on _job["rt"].
♻️ Duplicate comments (2)
api/base.py (2)
314-316:⚠️ Potential issue | 🟡 MinorStill logging the
Matchobject here.
re.match()returnsMatch | None, so this warning never prints the numeric distance. Extractgroup(1)after aNonecheck.🐛 Proposed fix
pattern = r"[^0-9\.]*(.+)米[^0-9\.]*" msg = re.match(pattern, resp_txt) - logger.warning(f"距离签到位置 {msg}m") + if msg: + logger.warning(f"距离签到位置 {msg.group(1)}m") + else: + logger.warning(f"无法解析签到距离: {resp_txt}")#!/bin/bash python - <<'PY' import re resp_txt = "距离签到位置 12.5米" msg = re.match(r"[^0-9\.]*(.+)米[^0-9\.]*", resp_txt) print(msg) print(msg.group(1) if msg else None) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/base.py` around lines 314 - 316, The warning currently logs the Match object returned by re.match instead of the captured numeric string; in the block where pattern = r"[^0-9\.]*(.+)米[^0-9\.]*" and msg = re.match(pattern, resp_txt), check if msg is not None and then extract the captured group via msg.group(1) (or set a default like None/empty if no match) and pass that value into logger.warning (e.g., logger.warning(f"距离签到位置 {distance}m")) so the numeric distance is printed rather than the Match object.
512-512:⚠️ Potential issue | 🟠 MajorThis non-200 log path still raises before it logs.
"刷新视频状态返回码异常: {}" % resp.status_codehas no%placeholder, so Python raisesTypeErrorand_refresh_video_status()never reaches the intendedreturn Noneon this branch. Use Loguru's brace formatting with a separate argument.🐛 Proposed fix
- logger.debug("刷新视频状态返回码异常: {}" % resp.status_code) + logger.debug("刷新视频状态返回码异常: {}", resp.status_code)#!/bin/bash python - <<'PY' try: print("刷新视频状态返回码异常: {}" % 403) except Exception as exc: print(type(exc).__name__, exc) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/base.py` at line 512, The log call in _refresh_video_status uses Python’s percent-formatting incorrectly and raises before returning; change the logger.debug invocation so it uses Loguru’s brace-style formatting with the status code as a separate argument (i.e. pass the message template "刷新视频状态返回码异常: {}" and resp.status_code as the next parameter) so the log prints the status without raising and the function can return None as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/base.py`:
- Around line 256-262: The get_activity_list() function currently assumes
resp.json() returns the expected structure and will raise on invalid JSON or
missing keys; wrap the JSON decode and subsequent key access in a try/except
that catches JSON decode errors (ValueError/JSONDecodeError) and
KeyError/TypeError, log the exception with context (including resp.url or
truncated resp.text at debug level), and return [] as a safe fallback; validate
that data is a dict and contains "result" and nested "data" -> "activeList"
before returning it.
---
Outside diff comments:
In `@api/base.py`:
- Around line 602-607: The refreshed_meta handling currently assigns to
throwaway locals and leaves loop state stale; in the recovery branch update the
actual loop variables by assigning _dtoken = refreshed_meta["dtoken"], duration
= refreshed_meta["duration"], and play_time = int(refreshed_meta["playTime"])
(or coerce/validate numeric) instead of using .get fallbacks, and if any of
those keys are missing raise/log an error rather than silently falling back;
update the values before the logger.debug call so the next iteration uses the
refreshed state.
- Around line 391-417: The fast-path sets _isdrag but never updates
params["rt"], so when callers pass _isdrag=4 the request still uses _job["rt"]
or the old fallback; update the params construction (the params dict creation
where "isdrag" and "view" are set) to explicitly set params["rt"] based on the
_isdrag argument (e.g., override params["rt"] = _isdrag when _isdrag is provided
or when it indicates the seekable/fast-path value) instead of relying solely on
_job["rt"].
---
Duplicate comments:
In `@api/base.py`:
- Around line 314-316: The warning currently logs the Match object returned by
re.match instead of the captured numeric string; in the block where pattern =
r"[^0-9\.]*(.+)米[^0-9\.]*" and msg = re.match(pattern, resp_txt), check if msg
is not None and then extract the captured group via msg.group(1) (or set a
default like None/empty if no match) and pass that value into logger.warning
(e.g., logger.warning(f"距离签到位置 {distance}m")) so the numeric distance is printed
rather than the Match object.
- Line 512: The log call in _refresh_video_status uses Python’s
percent-formatting incorrectly and raises before returning; change the
logger.debug invocation so it uses Loguru’s brace-style formatting with the
status code as a separate argument (i.e. pass the message template "刷新视频状态返回码异常:
{}" and resp.status_code as the next parameter) so the log prints the status
without raising and the function can return None as intended.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements