Skip to content

fix(test): two dashboard-api reds — conversions changed the log CHANNEL and the error MESSAGE - #2774

Merged
gsxdsm merged 2 commits into
mainfrom
fix/dashboard-api-curated-reds
Jul 30, 2026
Merged

fix(test): two dashboard-api reds — conversions changed the log CHANNEL and the error MESSAGE#2774
gsxdsm merged 2 commits into
mainfrom
fix/dashboard-api-curated-reds

Conversation

@gsxdsm

@gsxdsm gsxdsm commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Both red on main. api:curated goes 2 failed → 34 files / 1599 passed. Neither is a product defect — both are conversions the tests had not followed.

1. The log channel moved

sse.test.ts spied console.log. sseDebug routes through createLogger("sse").debug (sse.ts:50-53), and the shared logger writes debug lines to console.error carrying a \0fnlvl=info\0 severity marker — that is the point of FN-8603's adapter.

So the spy saw nothing, and the failure read expected false to be true, naming neither the channel nor the logger. The stderr in the run output showed the lines being emitted the whole time:

fnlvl=info [sse] [sse] + connection (active=1, hwm=2)
fnlvl=info [sse] [sse] - connection (active=0)

2. The error message is now built from resolved lanes

routes-tasks asserted the substring "in-review or in-progress". The message is now:

const allowed = [...prFeedbackReviewColumns, prFeedbackWipColumn]
  .map((column) => `'${column}'`).join(" or ");
throw badRequest(`PR feedback can only be addressed for tasks in ${allowed}`);

so it reads 'in-review' or 'in-progress' — quoted, and derived from the resolved columns.

Asserted each lane separately rather than re-pinning the joined string. The join order and separator are presentation; the lanes being the resolved review + wip columns is the fact this case owns. Re-pinning the punctuation would break again on the next formatting change and would not have caught a wrong lane — which is the failure this test exists to catch on a renamed board.

Verification

check result
test:quality:api:curated 2 failed → 34 files / 1599 passed
sse.test.ts 24 passed
routes-tasks.test.ts 99 passed
pnpm lint, dashboard tsc clean

Scope

Fix-forward only, per the u9 lane. Found by re-scanning the packages after #2739 / #2744 / #2754 merged, rather than by waiting for a report.

For the record on the other groups at the same commit: components-a 1195 passed, core is 2 failed — both already accounted for (archived-column-gate-parity is #2768's target, agent-logs-and-monitor.pg is the deferred funnel/analytics decision on #2669).

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@gsxdsm, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 7 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b99dd417-1571-4887-9231-69a60bde7c23

📥 Commits

Reviewing files that changed from the base of the PR and between 7119432 and 27a6e01.

📒 Files selected for processing (1)
  • packages/dashboard/src/__tests__/sse.test.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread packages/dashboard/src/__tests__/sse.test.ts Outdated
@greptile-apps

greptile-apps Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR updates SSE logging tests to follow the shared logger’s console.error channel and moves console-spy cleanup into afterEach so failed assertions cannot leak mocked global state.

  • Changes the debug-log assertions to inspect console.error.
  • Restores all spies after each test in the logging-severity describe block.
  • Removes manual restoration calls that could be skipped after assertion failures.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the previous console-spy leak is fixed by unconditional cleanup in the describe block’s afterEach hook.

Important Files Changed

Filename Overview
packages/dashboard/src/tests/sse.test.ts Updates SSE logger assertions and safely restores the describe block’s per-test console spies after every test.

Reviews (3): Last reviewed commit: "test(dashboard): restore console spies i..." | Re-trigger Greptile

@gsxdsm

gsxdsm commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

Already done on this branch — this looks like a re-report against an earlier revision.

sse.test.ts:432-445 has exactly the hook this asks for: an afterEach inside describe("createSSE connection log severity"), the block that encloses both spy-using tests (the describe opens at line 425), calling vi.restoreAllMocks(). Neither test carries a trailing mockRestore() any more, so there is nothing left for a failing assertion to skip.

The reasoning is recorded at the hook, including the part that makes this worth fixing rather than a style nit: these spy console.error, which is where the shared logger writes ALL diagnostics, so a leaked spy silences exactly the output you would need to debug the failure that caused the leak.

The hook also covers any case added to that describe later, which the per-test restore could not.

gsxdsm added 2 commits July 30, 2026 08:39
…HANNEL and the MESSAGE

Both red on main; api:curated 2 failed -> 34 files / 1599 passed.

1. sse.test.ts spied `console.log`. `sseDebug` routes through `createLogger("sse").debug`
   (sse.ts:50-53) and the shared logger writes debug lines to console.ERROR with a
   `\0fnlvl=info\0` severity marker — that is the point of FN-8603's adapter. The spy saw nothing
   and the failure read "expected false to be true", naming neither the channel nor the logger.

2. routes-tasks asserted the substring "in-review or in-progress". The message is now built from
   RESOLVED lanes and quotes each one — `[...prFeedbackReviewColumns, prFeedbackWipColumn]
   .map((c) => `'${c}'`).join(" or ")` (register-task-workflow-routes.ts:5969) — so it reads
   `'in-review' or 'in-progress'`.

   Asserted each lane separately rather than re-pinning the joined string: the join order and
   separator are presentation, while the lanes being the resolved review + wip columns is the fact
   this case owns. Re-pinning the punctuation would break again on the next formatting change and
   would not have caught a wrong lane.

Neither is a product defect — both are conversions the tests had not followed. Lint clean, dashboard
tsc clean.

Fusion-Task-Id: U9
…ptile P2)

Correct, and my change made the leak worse rather than introducing it. A failing assertion skips the
trailing `mockRestore()`, leaving the console mocked for every later test in the file — and since
these now spy `console.error`, which is where the shared logger writes ALL diagnostics, the leak
silences exactly the output needed to debug the failure that caused it.

Both cases in the describe had that shape (the `console.log` sibling too), so the restore moved into
the existing `afterEach` alongside the FUSION_DEBUG restore, and the two trailing manual calls are
gone. The hook also covers any case added later, which the per-test form never did.

MEASURED — forced a failure past where the old manual restore sat:
  before: the spy would remain installed for the rest of the file
  after:  1 failed | 23 passed, no cascade — later cases still see console.error
  as shipped: 24 passed; api:curated 34 files / 1599 passed

Lint clean.

Fusion-Task-Id: U9
@gsxdsm
gsxdsm force-pushed the fix/dashboard-api-curated-reds branch from a0f5b70 to 27a6e01 Compare July 30, 2026 15:40
@gsxdsm
gsxdsm merged commit c428eed into main Jul 30, 2026
7 checks passed
@gsxdsm
gsxdsm deleted the fix/dashboard-api-curated-reds branch July 30, 2026 16:22
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.

1 participant