Skip to content

fix(zhihu): make question runtime-compatible#732

Merged
jackwener merged 5 commits intojackwener:mainfrom
KehaoC:fix/zhihu-question-runtime
Apr 4, 2026
Merged

fix(zhihu): make question runtime-compatible#732
jackwener merged 5 commits intojackwener:mainfrom
KehaoC:fix/zhihu-question-runtime

Conversation

@KehaoC
Copy link
Copy Markdown
Contributor

@KehaoC KehaoC commented Apr 3, 2026

Summary

Fix zhihu question so it works in the real browser runtime again.

The command was using function-style page.evaluate(...), but the runtime only supports string-based evaluate. This PR switches it to the supported form, navigates to the question page first, and keeps the existing auth / fetch error mapping.

Validation

  • npx vitest run src/clis/zhihu/question.test.ts
  • npm run build
  • node dist/main.js zhihu question 2021881398772981878 --limit 2 -f json
    Uploading 截屏2026-04-03 17.10.32.png…

Closes #604

@KehaoC
Copy link
Copy Markdown
Contributor Author

KehaoC commented Apr 3, 2026

截屏2026-04-03 17 10 32

Sorry, here is the image when running.

- Build URL in Node.js, embed via JSON.stringify for safety-by-design
- Remove unnecessary (page as any) cast — IPage already has evaluate
- Simplify error message construction (no nested ternaries)
- Replace implementation-detail test with numeric ID validation test
Copy link
Copy Markdown
Contributor

@Astro-Han Astro-Han left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the runtime-compatibility change for zhihu question. I checked the string-based evaluate migration, the added numeric questionId guard, and the updated unit tests. I did not find blocking issues. I also ran npx vitest run src/clis/zhihu/question.test.ts locally on the PR branch, and all 5 tests passed.

@Astro-Han
Copy link
Copy Markdown
Contributor

Minor: error message formatting for fetch exceptions

When status === 0 and error is present, the current message reads:

Zhihu question answers request failed Failed to fetch

Two consecutive "failed" without a separator reads awkwardly. Suggested fix in question.ts:

const detail = result?.status > 0
  ? `with HTTP ${result.status}`
  : (result?.error ? `: ${result.error}` : '');

This produces Zhihu question answers request failed: Failed to fetch, which is clearer. The corresponding test expectation in question.test.ts should also be updated to match.

Otherwise the PR looks solid — runtime fix is correct, input validation and security hardening are well done. 👍

"request failed Failed to fetch" → "request failed: Failed to fetch"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

zhihu question command fails with 'Not logged in' despite valid session

3 participants