fix(ui): load all paginated projects (#1812) - #1819
Conversation
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
rarepops
changed the title
bda9282 to
f7bccab
Compare
|
CI note: the fresh run's The raw job log shows:
The branch is current with
I do not have permission to rerun repository jobs. A maintainer rerun of |
DeusData
added
bug
|
Thank you for the reproduce-first pagination coverage and for bounding schema hydration concurrency while publishing the project list early. The contribution queue is quite full, so the full review may take a little time. We have the PR routed and will come back with grounded feedback as soon as possible. |
|
Approved. The pagination loop is written with the right posture, which is what makes this safe rather than merely correct. You treat the server's own pagination metadata as untrusted. That is the part I would have asked for: const pageOffset = Number.isSafeInteger(page.offset) ? page.offset! : offset;
const returned = Number.isSafeInteger(page.returned) ? page.returned! : pageProjects.length;
const nextOffset = pageOffset + returned;
if (returned <= 0 || nextOffset <= offset) {
throw new Error("list_projects pagination did not advance");
}Two distinct ways a Publishing the complete list before schema hydration finishes is the right ordering: the thing the user asked for appears as soon as it is known, and the enrichment fills in behind it. Capping hydration at four concurrent requests keeps that from turning a 51-project workspace into 51 simultaneous calls. And the regression is sized to the bug — 51 projects against a 50-item page is exactly the boundary that was broken, rather than a round number that happens to exceed it. Your CI red is not yours
Everything downstream then skipped on a failed precondition — including I have now confirmed this identical cluster, with that identical diagnostic, on five unrelated pull requests spanning several days. It is ours to fix. Please do not chase it. On the unticked boxes: not running the full native suite or Please rebase — |
Follow list_projects pagination metadata so the management UI does not stop at 50 projects. Publish the complete list before schema hydration and cap schema lookups at four concurrent requests. Signed-off-by: Rares Popa <2606875+rarepops@users.noreply.github.com>
f7bccab to
64ea496
Compare
|
This one needs an honest explanation, because it was approved and was next in the merge order. Since then a direction decision has landed: the graph UI is being reimplemented from the ground up, and the current React app — including the projects hook this fixes — will be replaced wholesale rather than maintained. Merging into a surface that is about to disappear gives the fix a very short life, so we are closing this and #1812 as superseded rather than landing it and deleting it within a release cycle. What carries forward is the posture rather than the code: treating the server's own pagination metadata as untrusted, bounding the hydration concurrency, and publishing the list early are going in as requirements for the new projects view, with this PR as the reference. Thank you for a reproduce-first fix with more test than change — that is the shape we like — and for the careful CI attribution on the Windows guards leg, which was correct. You are very welcome back. |
…istic lock_registry_absolute_deadline_survives_repeated_wakes has failed on the test-unix (macos-15-intel) leg of three unrelated pull requests in one week -- #1342 (08-28), #1811 (09-02) and #1819 (09-03, run 33799475629, job 100823626965) -- always with the same signature: FAIL tests/test_lock_registry.c:1153: ASSERT(tail_queued) 7648 passed / 1 failed The registry is not racy. cbm_lock_registry_acquire enqueues the waiter synchronously under the registry mutex before any wait, so waiter_count and the attempting count are exact. The defect is in the fixture: it raced two wall-clock windows against each other, both anchored to a timestamp the observer thread took before the tail thread had even been scheduled. deadline_start = cbm_now_ms(); tail.deadline_ms = deadline_start + 200; /* the tail's acquire deadline */ queue_deadline = deadline_start + 100; /* the observer's budget */ Because both windows start before the tail runs, a loaded runner breaks the fixture two different ways: * the tail is scheduled inside its deadline but after the observer's 100 ms budget has expired -- the queued state existed and was simply no longer being looked at; or * the tail is scheduled more than 200 ms late, in which case its deadline has already passed when it finally calls acquire, the pre-registration deadline check in lock_registry_acquire_internal returns BUSY immediately, and the tail never enqueues at all -- the asserted state can then never occur, however long the observer waits. lock_registry is in the parallel wave of run-tests-parallel.sh, not the serial tail, so on a small CI runner it competes with a full wave of sanitized suites -- exactly the scheduling delay both paths need. Widening the observer's budget would only paper over the first path, and the state it waits for is transient by construction: it exists only between the tail's enqueue and the tail's own deadline. So the fixture is rebuilt to observe states that cannot evaporate. * The tail anchors its absolute deadline itself, in its own thread, immediately before the acquire it bounds. Scheduling delay can no longer consume the deadline before the call starts, so the enqueue is unconditional, and elapsed is measured from the tail's own anchor -- it now times the registry instead of timing the scheduler. * Enqueue is exposed as a monotonic counter, cbm_lock_registry_waiter_enqueue_count_for_test, next to the existing test_condition_wait_calls counter it is modelled on. Because the count only ever grows, the observer reads it once after the tail has returned rather than trying to catch a live queue depth: the polling loop, and with it the window, is gone. * The fixture's remaining 500 ms and 600 ms budgets become the file's LOCK_REGISTRY_TEST_TIMEOUT_MS backstop, and each loop exits on the state it waits for instead of on the clock, so the backstop only fires when the product is actually broken. The contract is unchanged: the tail must still return at its absolute deadline (150 <= elapsed < 350 ms for a 200 ms deadline) despite ~40 unrelated cancel broadcasts, still with BUSY and no lease, with the head still holding the attempt. The broadcast loop now starts immediately after the tail is released, so it overlaps the tail's wait at least as much as it did before. Verification, all on macOS arm64 with the sanitized runner: * The mechanism was reproduced locally by delaying only the tail thread after its start gate, with production untouched. A 120 ms delay (the state exists, outside the observer's budget) and a 250 ms delay (the tail never enqueues) each produced exactly one failure, ASSERT(tail_queued) -- the CI signature. * After the rebuild the same injections are green at 120, 250, 400 and 900 ms: an arbitrary scheduling delay no longer decides the verdict. * lock_registry 30/30 green plain and 30/30 green under four CPU hogs; 16/16 inside the saturated 18-job parallel wave. * private_file_lock, lock_registry, daemon, project_lock and the daemon_* suites: 222 passed, 1 skipped. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
2 participants
What does this PR do?
Fixes #1812.
list_projectspagination untilhas_moreis falseValidation
cd graph-ui && npm test(41 passed)cd graph-ui && npm run buildgit diff --checkChecklist
git commit -s)make -f Makefile.cbm test) (full native suite not run; focused UI suite passed)make -f Makefile.cbm lint-ci) (not run locally)