Skip to content

[api] update getHistory to call history_v2 #4389

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

[api] update getHistory to call history_v2 #4389

wants to merge 3 commits into from

Conversation

ric-yu
Copy link

@ric-yu ric-yu commented Jul 8, 2025 •
edited
Loading

Summary

Updates the history API to use a cleaner array format where each history item contains its prompt_id as a property rather than being wrapped in an
object with the prompt_id as a key.

Backend PR: comfyanonymous/ComfyUI#8844

Changes Made

API Response Format Update

  • Before: {history: [{"prompt_id_1": {...}}, {"prompt_id_2": {...}}]}
  • After: {history: [{prompt_id: "prompt_id_1", ...}, {prompt_id: "prompt_id_2", ...}]}

Schema Changes (src/schemas/apiSchema.ts)

  • Added prompt_id field to zRawHistoryItem schema
  • Updated zHistoryResponse to expect a direct array of history items instead of an array of objects with dynamic keys
  • Added proper TypeScript types for the new response format

API Implementation (src/scripts/api.ts)

  • Updated getHistory() method to use the new /history_v2 endpoint
  • Simplified data extraction logic to directly map the history array
  • Removed unnecessary object key extraction since prompt_id is now a direct property
  • Removed unused RawHistoryItem import

Test Coverage (tests-ui/tests/scripts/api.test.ts)

  • Added comprehensive test suite for the new history API format
  • Tests cover:
    • Array format handling from /history_v2
    • Empty history arrays
    • Error handling
    • Correct API endpoint usage
    • Default parameter behavior

Test Results

All tests pass successfully, confirming the API correctly handles the new format while maintaining backward compatibility for error scenarios.

image

@ric-yu ric-yu requested a review from guill July 8, 2025 20:15
@ric-yu ric-yu self-assigned this Jul 8, 2025
@ric-yu ric-yu requested review from a team as code owners July 8, 2025 20:15
@christian-byrne
Copy link
Contributor

We can definitely upgrade vite. @jtydhr88 were you able to test vite 7 yet?

@ric-yu ric-yu force-pushed the update-history-api-to-array branch 3 times, most recently from d29562a to 1b15e98 Compare July 8, 2025 21:31
@ric-yu
Copy link
Author

ric-yu commented Jul 9, 2025

We can definitely upgrade vite. @jtydhr88 were you able to test vite 7 yet?

Makes sense, I'll leave it outside the scope of this PR for now, since it required a change in vite.config.mts

@ric-yu ric-yu force-pushed the update-history-api-to-array branch from 1b15e98 to aabadad Compare July 9, 2025 00:58
@jtydhr88
Copy link
Collaborator

jtydhr88 commented Jul 9, 2025

@christian-byrne previously I tested on vite 6.3.5, and it needs some dependencies upgrade as well, I could send a draft PR for your reference and testing

@ric-yu ric-yu changed the title [api] update get history to take an array [api] update prompt queue to call history_v2 Jul 9, 2025
@ric-yu ric-yu changed the title [api] update prompt queue to call history_v2 [api] update getHistory to call history_v2 Jul 9, 2025
christian-byrne
christian-byrne reviewed Jul 9, 2025
View reviewed changes
src/scripts/api.ts
@@ -710,13 +712,21 @@ export class ComfyApi extends EventTarget {
max_items: number = 200
): Promise<{ History: HistoryTaskItem[] }> {
try {
const res = await this.fetchApi(`/history?max_items=${max_items}`)
const json: Promise<HistoryTaskItem[]> = await res.json()
const res = await this.fetchApi(`/history_v2?max_items=${max_items}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this endpoint be added to open-source ComfyUI core server?

Copy link
Author

Choose a reason for hiding this comment

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

Yessir comfyanonymous/ComfyUI#8844

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay nice. For the discovery, you can also access the version of ComfyUI via /system_stats or the systemStatsStore in our codebase. In general we don't worry about supporting old versions of the backend (relative to frontend version) since comfyui_frontend_package is a dependency of ComfyUI and not used with any other backends. However, I may be missing some context.

@ric-yu ric-yu force-pushed the update-history-api-to-array branch from aabadad to 2ed47e1 Compare July 9, 2025 17:53
@ric-yu ric-yu mentioned this pull request Jul 9, 2025
Closed
@ric-yu ric-yu force-pushed the update-history-api-to-array branch from 252b0b6 to 65464c4 Compare July 9, 2025 20:24
@ric-yu ric-yu requested a review from comfyanonymous July 9, 2025 21:22
webfiltered
webfiltered previously approved these changes Jul 9, 2025
View reviewed changes
Copy link
Contributor

@webfiltered webfiltered left a comment

Choose a reason for hiding this comment

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

LGTM.

The only feedback I have is that it wasn't immediately obvious what the actual goal of the PR was - but the link to core PR took care of that. 👍

src/locales/en/settings.json Outdated
Comment on lines 235 to 238
"Comfy_Notification_ShowVersionUpdates": {
"name": "Show version updates",
"tooltip": "Show updates for new models, and major new features."
},
Copy link
Contributor

Choose a reason for hiding this comment

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

@ric-yu Feel free to mark as resolved if already resolved - I'm sure I saw this brought up somewhere else.

If not, @christian-byrne can you please confirm this can be ignored after your CI changes?

@christian-byrne
Copy link
Contributor

Backend change needs to happen first

@webfiltered
Copy link
Contributor

Backend change needs to happen first

And this does higlight the need for integration tests again. Issue'd.

@ric-yu
Copy link
Author

ric-yu commented Jul 9, 2025

Closing - will just sort history by exec start

@ric-yu ric-yu closed this Jul 9, 2025
@ric-yu ric-yu reopened this Jul 10, 2025
@ric-yu ric-yu marked this pull request as draft July 11, 2025 21:04
@ric-yu ric-yu force-pushed the update-history-api-to-array branch from 65464c4 to 9a6a1c1 Compare July 12, 2025 20:31
@ric-yu ric-yu force-pushed the update-history-api-to-array branch from 0371a2a to 2b72d18 Compare July 12, 2025 22:15
@ric-yu ric-yu marked this pull request as ready for review July 12, 2025 23:14
@ric-yu ric-yu force-pushed the update-history-api-to-array branch from 2b72d18 to 6153e06 Compare July 12, 2025 23:14
@ric-yu ric-yu force-pushed the update-history-api-to-array branch from 6153e06 to 75faab2 Compare July 13, 2025 06:19
@christian-byrne
Copy link
Contributor

Stop force-pushing

@ric-yu ric-yu marked this pull request as draft July 14, 2025 20:58
@christian-byrne christian-byrne added the Public API Affects or interacts with the public API surface (affecting custom node or extension authors) label Aug 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christian-byrne christian-byrne christian-byrne left review comments

@webfiltered webfiltered webfiltered left review comments

@guill guill Awaiting requested review from guill

@Yorha4D Yorha4D Awaiting requested review from Yorha4D

@KarryCharon KarryCharon Awaiting requested review from KarryCharon

@shinshin86 shinshin86 Awaiting requested review from shinshin86

@comfyanonymous comfyanonymous Awaiting requested review from comfyanonymous

At least 1 approving review is required to merge this pull request.

Assignees

@ric-yu ric-yu

Labels
Public API Affects or interacts with the public API surface (affecting custom node or extension authors)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ric-yu @christian-byrne @jtydhr88 @webfiltered