Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Long resp handling #401

Open
wants to merge 67 commits into
base: master
Choose a base branch
from
Open

Long resp handling #401

wants to merge 67 commits into from

Conversation

MrOrz
Copy link
Member

@MrOrz MrOrz commented Jan 30, 2025

Fixes #395

  • Implements a new reply token management mechanism to handle long-running queries.
  • Adds utility function setReplyTokenCollectorMessage(ctx: Context, message: string) to tell the user about current progress on timeout
    • Ensures that before the 1-minute timeout, the bot sends a message with a quick reply to collect a new reply token.
  • Modifies singleUserHandler to remove the isRepliedDueToTimeout flag.
  • Updates the send() function to consume the reply token if it hasn't timed out or use a push message if the token has expired.

Updates on tests

  • For handlers that calls displayLoadingAnimation, mock lineClient to suppress the warning caused by sending request with invalid token to LINE server.
  • setReplyToken() timeout mechanism is tested in src/webhook/handlers/__tests__/utils.test.js
  • For handlers that imports webhook/handlers/utils directly or indirectly:
    • handler implementations usually imports from webhook/handlers/utils; we add redis.quit() in setup-jest to stop the Redis connection to avoid open handles caused by Redis connection.
    • If a test file mocks other handlers, implement the mock with jest.fn() so that we don't accidentally create connections to Redis in the namespace for mocks
      • It seems that the redis client instance in mock namespace and test files (setup-jest) are separate, so redis.quit() in setup-jest does not break the connection when redis client is instantiated under mocks.

Other refactors

  • Prepend [...] to logging to identify where a certain error message is from

Screenshots

process media route

Send 1 message

screen-20250131-190104.2.mp4

process batch route

Send multiple messages

multi-msg.mp4

asking article submission

Send new message

screen-20250131-201953.mp4

@MrOrz MrOrz force-pushed the long-resp-handling branch from ccb77b2 to 3656927 Compare January 30, 2025 03:34
@MrOrz MrOrz changed the base branch from master to fix-ai-reply-name January 30, 2025 03:34
@MrOrz MrOrz force-pushed the long-resp-handling branch from 3656927 to 753b089 Compare January 30, 2025 03:41
Base automatically changed from fix-ai-reply-name to master January 30, 2025 13:32
@coveralls
Copy link

coveralls commented Jan 31, 2025

Pull Request Test Coverage Report for Build 13479682573

Details

  • 66 of 68 (97.06%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.4%) to 90.989%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/webhook/handlers/processBatch.ts 0 1 0.0%
src/webhook/handlers/singleUserHandler.ts 15 16 93.75%
Totals Coverage Status
Change from base Build 13053493535: 0.4%
Covered Lines: 1125
Relevant Lines: 1192

💛 - Coveralls

@MrOrz MrOrz marked this pull request as ready for review January 31, 2025 10:49
@MrOrz MrOrz self-assigned this Jan 31, 2025
@MrOrz MrOrz force-pushed the long-resp-handling branch from 534cd0e to 49913e7 Compare February 23, 2025 04:27
@MrOrz MrOrz mentioned this pull request Feb 23, 2025
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.

Handle long requests gracefully
6 participants