feat(reporting): build Stage 09 reporting foundations - #47
Open
Andreas-Froyland wants to merge 340 commits into
Open
Andreas-Froyland wants to merge 340 commits into
Andreas-Froyland wants to merge 340 commits into
Conversation
Posts to the SUP-02-8 messages endpoint with kind outgoing or note. Stored only -- nothing is emailed until Stage 04, and the composer says so rather than implying a send. Mode is signalled by the container, the tabs, the placeholder, the caption, and the submit button, not by one highlighted tab: in note mode the whole strip turns amber with a lock icon, reads "Only your team will see this", and the button becomes "Add internal note". Same visual language as the note bubbles in SupportMessageItem. Acceptance criterion 4 treats an agent posting a note as a public reply as the worst failure in a support tool, so the signals are deliberately redundant. The draft and the mode both reset when the selected conversation changes -- carrying a note draft into another ticket risks posting it to the wrong one. Mode persists after a successful post, since agents add notes in runs and the strip stays visibly amber throughout. Also drops the composer slot wrapper's padding (the composer owns its own, so note mode tints the full width) and clears two now-stale comments about endpoints that had not landed yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`yarn format:check` reports 485 failing files locally on Windows, but 445 of those are an artifact of core.autocrlf=true: the working tree is CRLF while .prettierrc.json sets no endOfLine, so Prettier defaults to "lf" and flags every file. The index stores LF for all 1000+ tracked files (0 with CRLF), so CI checks out LF and never sees those. The real CI-failing set is 40 files, found with `prettier --check --end-of-line auto`, which ignores line endings and reports only genuine formatting drift. Those are what this commit fixes; nothing else is touched. Verified as CI sees it: each of the 40 files' index (LF) content piped through `prettier --check --stdin-filepath` under default settings passes, as does a sample of unchanged files. harness:verify green. Note this adds one lint warning (159, still 0 errors): Prettier rewrites `<input ...>` to `<input ... />` in pages/feedback/[id]/index.vue, which eslint-plugin-vue's html-self-closing rule warns about. That is a standing Prettier/ESLint disagreement about void elements, not something this commit introduced conceptually - it will recur whenever a Vue file with void elements is formatted. Resolving it means aligning the two configs, which is a repo-wide decision rather than part of a formatting pass.
SUP-02-13 asked to hide the nav, stop inbound processing, and preserve data when a team disables the Support module. The nav half shipped with SUP-02-12 and preserving data needed no work, but there is no inbound processing until Stage 03 -- a guard written now would sit against a code path nothing exercises and could not be tested, which is how isUniqueViolation() stayed silently broken (D-24). Moved to stage-03-inbound-email.md with its own acceptance criterion: record the event, return 200, create no conversation, never 404. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Create a conversation, reply, add an internal note, change status, and confirm the change rendered into the thread as an activity message from the same ordered query. Asserts isPrivate is false on the reply and true on the note -- the server derives it from kind, so this exercises the guard rather than the client's request -- and that re-sending an unchanged status appends no phantom activity message. The spec could not be executed: any Playwright spec importing db dies at collection on a consola export-condition mismatch (delta D-33, queued as SUP-X-5). Pre-existing -- Stage 01's support-contact-timeline.spec.ts fails identically. Every assertion was verified by hand against a live dev server and database instead, then the test data removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Stage 02 is complete, so Stage 03 enters TODO.md as SUP-03-1..14 per the dispatch protocol. Rewrites parallel-agents.md for Stage 03. The split differs from Stage 02's API/UI seam because SUP-03-4 (the inbound endpoint) consumes almost every other item -- threading, stripping, sanitization, auto-response detection, contacts, attribution, attachments all meet inside it. The seam is pipeline vs pure modules instead: Agent 1 owns the wire and the endpoint, Agent 2 owns the content modules it calls plus rendering. That only works if both sides agree signatures up front, so the doc pins InboundMessage, resolveThread, stripQuotedReply, sanitizeInboundHtml, and isAutoResponse before either side writes code. Stage 02 shipped a feature that was correct on both sides and broken where they met; this stage has more seams, not fewer. Also assigns SUP-X-5 to Agent 2 as a precondition for SUP-03-14, since Playwright cannot currently collect any spec importing db -- which may mean two stages of "verified by E2E" criteria have been enforcing nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ght (SUP-X-5) Three specs could not be collected at all. The error surfaced only as "No tests found", which reads like a bad path rather than a broken import, and yarn harness:verify skips the E2E gate rather than failing it -- so this hid for two stages. Root cause was not the browser/node export split first assumed: both consola builds export createConsola. Playwright resolves the require condition to lib/index.cjs, which assigns exports in a dynamic loop, so cjs-module-lexer cannot see them and the ESM named import fails. Node and Nuxt resolve the .mjs build, so the app was never affected. Gives the suite its own client in tests/e2e/helpers/db.ts, built from pg plus the schema, which depends only on drizzle-orm/pg-core. No app module -- and so no logger, and no consola -- enters the test process. logger.ts is deliberately untouched: it is used app-wide and the interop shape differs between builds, so changing it to suit a test runner risked breaking production logging. Stage 01's cross-tenant isolation, concurrent-merge, and cursor pagination criteria were written as "verified by E2E" and were enforcing nothing. All five tests now collect: 3 pass, 2 skip on absent fixtures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…3-6, SUP-03-9) Two pure modules the inbound endpoint (SUP-03-4, other agent) consumes, built to the signatures pinned in parallel-agents.md. stripQuotedReply cuts at the earliest quote marker -- Gmail/Apple "On … wrote:" including the wrapped form clients emit, Outlook's divider and its no-divider From/Sent header block, localised dividers, forwarded blocks -- then drops trailing >-quoted lines and an RFC 3676 signature. Falls back to flattened HTML when there is no text part, so HTML-only senders do not produce empty messages. The untouched input always comes back as rawBody for conversationMessage.metadata, so a bad strip is a rendering annoyance recoverable from the record, not data loss. If a strip would empty the message the heuristics are assumed to have misfired and the full source is returned instead. isAutoResponse deliberately biases toward false negatives: a missed auto-reply is one junk message an agent deletes, a false positive silently discards a real customer email. Matches Auto-Submitted != no, the X-Autoreply family, X-Auto-Response-Suppress, a null return-path, and Precedence: auto_reply -- but NOT bulk or list, which customers forward routinely. 25 unit tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…03-1..03-3) server/services/support-channels/ mirrors the DOMAIN_PROVIDER adapter shape: types.ts defines the normalized InboundMessage, webhook/ holds the Postmark and Mailgun drivers, and index.ts selects one from SUPPORT_CHANNEL_PROVIDER. Nothing downstream learns which provider a message came from. InboundMessage matches the signature pinned in parallel-agents.md exactly, since the threading, quote-stripping, sanitization, and auto-response modules all consume it. The two drivers verify requests very differently, and the contract's "signature verification" wording hides it: **Postmark does not sign inbound webhooks at all**. Its documented protection is HTTP Basic Auth in the webhook URL plus IP allowlisting, so verifySignature there is a constant-time credential comparison. Mailgun does sign - HMAC-SHA256(timestamp + token) with the signing key - and additionally gets a 15-minute freshness window, because a signature over a fixed timestamp/token pair stays valid forever otherwise. Both fail closed when unconfigured; an empty credential must never mean "accept anything" on a publicly reachable intake endpoint. Unknown provider names resolve to null rather than a default driver, so /api/support/inbound/typo cannot be verified under the wrong scheme. Two deliberate choices worth knowing downstream: Mailgun's `stripped-text` is discarded in favour of the full body, because quote stripping is SUP-03-6 and must behave identically across providers; and idempotency keys on the Message-ID rather than Mailgun's signing token, which changes per retry and would defeat duplicate suppression. 26 unit tests against captured payload fixtures, covering both verification paths, replay rejection, attachment decoding, and filename path-traversal.
Decides whether inbound mail continues a conversation or starts one. The
failure is asymmetric -- a missed match splits a thread an agent can
merge, a wrong match shows one customer another's correspondence -- so
exact header matching runs before the subject heuristic.
1. In-Reply-To and every References entry against stored
channelMessageIds. Deliberately NOT contact-scoped: a CC'd
participant replying is a different contact on the same thread. It is
inbox-scoped, so a Message-ID cannot pull a message across teams.
2. The References root against conversation.channelThreadKey.
3. Subject heuristic, fenced four ways: same inbox, same contact, still
open or pending, and inside a 7-day window. The contact scope is what
stops two customers mailing "Invoice question" from landing in one
conversation.
normalizeSubject is exported and separately tested -- stacked prefixes,
numbered Re[2]:, localised AW/WG/SV/RES, and the case where a subject
merely starts with those letters ("Refund request" must survive).
Takes a structural ThreadableMessage rather than importing
InboundMessage, so server/utils does not depend on
server/services/support-channels. An InboundMessage satisfies it, so the
pinned call site compiles unchanged; flagged rather than changed
silently.
Also fixes the Postgres integration runner, which hardcoded
support-counter.test.ts and would have silently never run this suite --
the same class of gap as SUP-X-5. It now globs the directory and
excludes the Redis spec, which has its own runner.
13 unit tests, 10 against real Postgres.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
design.md requires both layers for inbound HTML -- sanitize on ingest AND sandbox on render -- because either alone is insufficient. This adds both. server/utils/inbound-sanitize.ts applies a strict allowlist built on sanitize-html rather than a hand-written one. The dangerous part is not parsing HTML, it is the long tail of bypasses: javascript: behind entity encoding, svg/math foreign-content quirks, mXSS on re-serialization, CSS expression(). A regex sanitizer fails to those. Adds sanitize-html as a direct dependency -- the first dependency change this stage, so worth noting for lockfile conflicts. Blocked: script, iframe, object, embed, form/input, style elements and attributes, link/meta/base, svg, and img -- a remote img src is a tracking pixel that fires when an agent opens a ticket and leaks their IP. Inline images arrive via Content-ID (SUP-03-8) and need cid: rewriting to our own storage, a deliberate decision for that item. Two policy gaps the tests caught, both real: - transformTags added rel/target, but allowedAttributes filtered them straight back out. - allowedSchemes only governs URLs that have a scheme, so a relative href like /settings survived -- and in the agent UI that resolves against our own origin, turning a hostile email into a link into the authenticated app. Relative hrefs are now dropped, text kept. SupportMessageHtml.vue renders bodyHtml in an iframe with sandbox="" -- no allow-scripts, no allow-same-origin -- so anything past the sanitizer cannot run, reach cookies, or navigate the tab. Never v-html. Height is measured on load with a fixed fallback, since an opaque-origin frame cannot be read; that is the sandbox working, not a bug. 18 unit tests covering the bypasses, not just <script>. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tly lost The auth forms are `<form @submit.prevent>` with `type="submit"` buttons, so until Vue hydrates and binds the listener a click performs a NATIVE form submission. With no `action` that is a GET to the same URL: the page reloads to `/login?`, whatever was typed is discarded, no request is made, and no error is shown. Nothing tells the person their sign-in did not happen. Credentials were never exposed by this - the inputs carry `id` but no `name`, and only named fields are serialized into a native submit, which is why the reload lands on a bare `/login?` with no query. The inputs need the same gate as the buttons, not just the buttons. Filling a field before hydration writes to the DOM while `v-model` state stays empty, so hydration discards the value and submit then fails validation with empty fields - the same defect one step earlier in the flow. Measured against a live server, clicking with zero settling wait: 0/5 sign-ins before, 5/5 after. This also makes scripted sign-in deterministic for free, because automation already waits for a control to be enabled - which is how the flake was found, as an intermittent redirect loop back to /login during Stage 02 browser testing. Scoped to the three cold-load entry points. The same pattern exists in settings, onboarding, and feedback-edit forms, but those are only reachable after client-side navigation, where the app is already hydrated.
Normalized InboundMessage, driver selection from SUPPORT_CHANNEL_PROVIDER, and the Postmark and Mailgun webhook drivers with signature verification. Also carries an out-of-stage auth fix (05f1b02) gating the auth forms on hydration.
resolveThread takes a structural ThreadableMessage so server/utils does not depend on server/services/support-channels. That is only safe if an InboundMessage really satisfies it -- and nothing calls resolveThread with one yet (the endpoint is SUP-03-4), so the seam would otherwise go unchecked until integration. Stage 02 shipped a feature correct on both sides and broken where they met, precisely because no check spanned the boundary. typecheck now fails if either side drifts. Also fixes a Transformer typing error in the sanitizer: the conditional attribute return produced a union sanitize-html would not accept. Landed after the last harness run, so typecheck caught it here rather than earlier. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-11, 03-12) POST /api/support/inbound/[provider], following the stage doc's order of operations exactly: verify signature, atomically claim, archive raw, parse, resolve inbox, resolve contact, resolve or create conversation, insert the message, stamp the event. Only a failed signature is a 401. Everything the endpoint deliberately declines - duplicate delivery, unknown recipient, disabled team, disabled inbox, auto-response - returns 200 with the event recorded, because a 4xx makes a mail provider retry the same message indefinitely. inbound-events.ts holds the claim as a single conditional upsert, so two concurrent deliveries of one email cannot both win. The ON CONFLICT WHERE clause reclaims only an unfinished event whose lease has lapsed: a live claim is never stolen, a processed one is never redone, and a crash mid-processing becomes replayable instead of wedging that email forever. Verified against real Postgres with a 12-way race admitting exactly one winner. Required making supportEmailEvent.inboxId nullable (migration 0024, delta D-34). The spec requires recording events for mail that matched no inbox and for teams with support disabled, which a NOT NULL column makes impossible; no ordering fixes it, since an unknown recipient never resolves to an inbox at all. Raised before being written, per the agent contract. Two fields exist purely to make the other agent's threading reachable, which reading their module surfaced: conversationMessage.channelMessageId carries the RFC Message-ID for their header match, and conversation.channelThreadKey carries the References root for their thread-key match. Neither is read by this endpoint - writing them wrong would have silently disabled threading rather than failing anything. Contact resolution keys on contactIdentity, not contact.email, so an alias does not open a second ticket, and follows mergedIntoContactId so mail never lands on a merge tombstone. CC participants exclude both the sender and the inbox's own addresses. Product attribution comes from the matched receiving address and is never overwritten on an existing conversation.
…UP-03-8) Attachments are stored before the transaction, since storage writes cannot roll back and the inline rewrite needs attachment ids before any row exists. A rolled back transaction therefore leaves orphaned objects rather than orphaned rows, which is the right way round - unreferenced bytes are collectable, a row pointing at nothing is not. A single failed upload drops that attachment rather than the email. Two caps: 10MB per part and 25MB per message, the second accumulated across parts so one message cannot arrive as a hundred large files. Inline rendering, which acceptance criterion 5 requires, needed three pieces. `cid:` references are rewritten to /api/support/attachments/<id> BEFORE sanitizing, so the sanitizer judges an ordinary same-origin path by its normal rules rather than being taught about `cid:`. The route serves bytes through the app, authorized via requireConversationAccess, so an attachment is exactly as reachable as its ticket and a guessed id gets a 403. Non-inline parts are sent as Content-Disposition: attachment with nosniff and a restrictive CSP, so an HTML or SVG attachment cannot execute in our origin. Crosses into inbound-sanitize.ts, which the agent contract assigns to Agent 2. Their own comment anticipated it - rendering inline images "means rewriting cid: to a URL served from our own storage, which is a deliberate decision for that item". `img` is now allowed, but only where a transform proves the src is our own attachment route; the check is anchored, so https://evil/api/support/attachments/x and //evil/... both still fail. Remote images stay blocked: they are tracking pixels that fire when an agent opens a ticket and leak their IP to the sender. Enabling img also exposed a real interaction bug in their emptiness check, which required text after stripping tags. That was correct while images were disallowed, but nulled an email whose entire body is a screenshot. A surviving img now counts as renderable content; an empty table shell still does not.
…-03-13) The item asked for a channel tab with provider selection, a webhook signing secret field, and a connection test. Two of those must not exist: SUP-03-1..03-3 put that config in the deployment environment, not per-inbox settings (delta D-34). A provider dropdown would imply a per-inbox choice that does not exist, and the app cannot write .env anyway. A secret field is worse -- it would either do nothing, or push a webhook credential into supportInbox.channelConfig where any team member can read and edit it. A real security regression traded for a form that looks complete. Built instead: GET /api/support/channel-status plus a read-only Channel card reporting which provider is selected, whether its driver resolves, whether its credentials are present, the NAMES of missing variables (never values), the webhook URL to register, and the address to point MX or a forwarding rule at. More useful than the connection test asked for: the realistic failure is not an unreachable provider, it is a deployment where the provider is set but credentials are not, so inbound mail is rejected and nothing in the product says so. The card reads "Not receiving mail" and names what to set. Verified live: 401 unauthenticated; authenticated correctly reports credentialsConfigured false with the two missing Postmark variables named on this dev box. Known duplication flagged in D-34: REQUIRED_ENV hard-codes each provider's variables, which belongs on ChannelDriver as isConfigured(). That file is the other agent's territory this stage, so it was flagged rather than edited. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…age-03 # Conflicts: # docs/plans/2026-08-11-support-platform/deltas.md
The inbound endpoint with supportEmailEvent claim/replay, contact and CC resolution, product attribution, the support-module switch, and attachment ingest with inline Content-ID rendering. Carries migration 0024, which drops NOT NULL from supportEmailEvent.inboxId (delta D-35). Correct: the event is claimed as soon as the signature verifies, before parsing reveals the inbox, and mail to an unrecognised address never resolves to one -- both of which Stage 03 requires recording. That NOT NULL was a defect in SUP-02-1's schema. Also carries a reviewed change to inbound-sanitize.ts allowing <img> only when the src has already been rewritten to our own access-checked attachment route.
…UTED Covers the Stage 03 acceptance criteria: a Postmark delivery creates a conversation, a reply with In-Reply-To threads onto the same one rather than opening a second, replaying the same providerEventId creates nothing further, and an unauthenticated delivery is rejected 401. Skips cleanly when SUPPORT_POSTMARK_WEBHOOK_USER/PASSWORD are unset, matching the guarded Redis and Postgres suites -- without credentials the endpoint rejects everything by design and there is nothing to assert. **This spec has not been run.** The dev server could not finish a cold start on this machine: single files were taking 180-320s to compile under contention from the second agent session. typecheck passes and harness:verify is green, but that proves the spec compiles, not that it passes. SUP-03-14 is deliberately left unchecked in TODO.md until someone runs it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-03-14) The spec asserted a conversation was created, but got 200 with `reason: "support-disabled"` and nothing created. That is the pipeline behaving correctly: `supportEnabled` defaults to false for every team (delta D-31) and SUP-03-10 honours it by recording the event, returning 200, and creating nothing. The spec never switched the module on, so it was asserting against a path that is supposed to create nothing. Diagnosed by replaying the spec's own three API calls by hand against a live server, since its `finally` block deletes the fixtures before anything can be inspected afterwards. `seed_preview_team` reports supportEnabled: false. The previous value is restored in `finally`. The seed team is shared with every other spec, so leaving Support switched on behind us would silently change what they exercise. Verified: this spec passes, and the full Playwright suite is 37 passed / 1 skipped / 0 failed. The remaining skip is the pre-existing local-storage-only upload test, which cannot run under STORAGE_DRIVER=s3. Crosses into tests/e2e/, which the agent contract assigns to Agent 2, but the spec shipped marked "NOT YET EXECUTED" and this was the single change needed to make it run.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 existing issues remain and 9 new issues found across 35 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/database/migrations/0043_petite_longshot.sql">
<violation number="1" location="server/database/migrations/0043_petite_longshot.sql:2">
P1: Custom agent: **Block Unsafe Database Migrations**
This migration blocks writes while it builds both unique indexes on `sla_target`. Use a phased rollout with `CONCURRENTLY` (and split the drop if needed) so the table stays writable during deployment.</violation>
<violation number="2" location="server/database/migrations/0043_petite_longshot.sql:2">
P1: On databases containing multiple catch-all targets for one policy and metric, this partial unique index creation fails because the previous index allowed duplicate NULL priority rows, aborting the migration. Deduplicate or clean up conflicting rows before applying the constraint.</violation>
</file>
<file name="server/utils/feedback-support-notifications.ts">
<violation number="1" location="server/utils/feedback-support-notifications.ts:62">
P2: When the subscriber email send fails, this condition still treats the address as delivered and suppresses the linked-contact fallback email. Track only successfully delivered subscriber addresses before applying this suppression, or retain the contact delivery when the subscriber send rejects.</violation>
</file>
<file name="server/services/rate-limit/index.ts">
<violation number="1" location="server/services/rate-limit/index.ts:38">
P2: When realtime and rate limiting are enabled in the same process, this creates a second regular-command Redis socket instead of using the shared publisher connection. That defeats the documented connection-count guarantee and can exhaust Redis client limits as instances scale; keep the limiter on a cached shared command connection, or add a cached shared variant that preserves the fail-fast options.</violation>
</file>
<file name="tests/e2e/anonymous-feedback.spec.ts">
<violation number="1" location="tests/e2e/anonymous-feedback.spec.ts:266">
P2: When the seeded team has Roadmap disabled, this setup does not enable it because it updates `project.settings` instead of the team module configuration. The test then cannot find the Roadmap link; enable the team’s roadmap module through its modules API or seed the required team state in the test fixture.</violation>
</file>
<file name="server/services/rate-limit/stores/redis.ts">
<violation number="1" location="server/services/rate-limit/stores/redis.ts:90">
P1: When a rate-limit EVAL times out, this branch disconnects the process-wide Redis client, interrupting unrelated commands that share its socket and potentially leaving the public API's Redis-backed features unavailable during reconnect. Do not reset a shared client from the store; use a dedicated connection or add coordinated reset logic at the shared-client owner.</violation>
</file>
<file name="lib/realtime-client.ts">
<violation number="1" location="lib/realtime-client.ts:186">
P2: When a tab becomes visible while an idle socket is still closing, `connect()` clears the idle close intent before the close callback runs. Keep idle-close state separate from the pagehide close intent so visible resumes reconnect without scheduling backoff.</violation>
</file>
<file name="server/services/domains/providers/static-cname.ts">
<violation number="1" location="server/services/domains/providers/static-cname.ts:32">
P2: The localhost shortcut only exists in registerProjectDomain, so a localhost custom domain flips back to dns_required on any re-check. getProjectDomainStatus (which verifyProjectDomain delegates to) performs dns.resolveCname, which cannot resolve .localhost per RFC 6761 and throws, so the catch returns buildPendingResult with verified=false and status='dns_required'. The verify flow (server/api/projects/[slug]/verify-domain.get.ts) then persists that result via buildDomainSettingsPatch/search persistProjectDomainResult, overwriting the active status and clearing domainVerifiedAt — undoing the localhost activation this change is meant to provide. Extract a shared localhost branch (e.g. buildLocalDevelopmentResult) and return it from getProjectDomainStatus before the DNS lookup as well.</violation>
</file>
<file name="server/api/support/teams/[teamId]/csat-summary.get.ts">
<violation number="1" location="server/api/support/teams/[teamId]/csat-summary.get.ts:89">
P3: The new 400 omits the structured error envelope that the rest of the support routes use for validation errors. Neighboring routes (e.g. `server/api/support/contacts/[id].put.ts:72`, `server/api/support/conversations/[id].patch.ts:90`, `server/api/support/canned-responses/[id].delete.ts`) throw `createError({ statusCode, statusMessage, data: createErrorResponse(ErrorCode.VALIDATION_ERROR, message) })`, so clients can branch on `data.error.code`. This response only carries `statusMessage`, so those clients receive no parsable error code. Align it with the existing pattern.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 5 unresolved issues already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| @@ -0,0 +1,3 @@ | |||
| DROP INDEX "sla_target_policy_metric_priority_idx";--> statement-breakpoint | |||
| CREATE UNIQUE INDEX "sla_target_policy_metric_catch_all_idx" ON "sla_target" USING btree ("sla_policy_id","metric") WHERE "sla_target"."priority" is null;--> statement-breakpoint | |||
There was a problem hiding this comment.
P1: Custom agent: Block Unsafe Database Migrations
This migration blocks writes while it builds both unique indexes on sla_target. Use a phased rollout with CONCURRENTLY (and split the drop if needed) so the table stays writable during deployment.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/database/migrations/0043_petite_longshot.sql, line 2:
<comment>This migration blocks writes while it builds both unique indexes on `sla_target`. Use a phased rollout with `CONCURRENTLY` (and split the drop if needed) so the table stays writable during deployment.</comment>
<file context>
@@ -0,0 +1,3 @@
+DROP INDEX "sla_target_policy_metric_priority_idx";--> statement-breakpoint
+CREATE UNIQUE INDEX "sla_target_policy_metric_catch_all_idx" ON "sla_target" USING btree ("sla_policy_id","metric") WHERE "sla_target"."priority" is null;--> statement-breakpoint
+CREATE UNIQUE INDEX "sla_target_policy_metric_priority_idx" ON "sla_target" USING btree ("sla_policy_id","metric","priority") WHERE "sla_target"."priority" is not null;
\ No newline at end of file
</file context>
| @@ -0,0 +1,3 @@ | |||
| DROP INDEX "sla_target_policy_metric_priority_idx";--> statement-breakpoint | |||
| CREATE UNIQUE INDEX "sla_target_policy_metric_catch_all_idx" ON "sla_target" USING btree ("sla_policy_id","metric") WHERE "sla_target"."priority" is null;--> statement-breakpoint | |||
There was a problem hiding this comment.
P1: On databases containing multiple catch-all targets for one policy and metric, this partial unique index creation fails because the previous index allowed duplicate NULL priority rows, aborting the migration. Deduplicate or clean up conflicting rows before applying the constraint.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/database/migrations/0043_petite_longshot.sql, line 2:
<comment>On databases containing multiple catch-all targets for one policy and metric, this partial unique index creation fails because the previous index allowed duplicate NULL priority rows, aborting the migration. Deduplicate or clean up conflicting rows before applying the constraint.</comment>
<file context>
@@ -0,0 +1,3 @@
+DROP INDEX "sla_target_policy_metric_priority_idx";--> statement-breakpoint
+CREATE UNIQUE INDEX "sla_target_policy_metric_catch_all_idx" ON "sla_target" USING btree ("sla_policy_id","metric") WHERE "sla_target"."priority" is null;--> statement-breakpoint
+CREATE UNIQUE INDEX "sla_target_policy_metric_priority_idx" ON "sla_target" USING btree ("sla_policy_id","metric","priority") WHERE "sla_target"."priority" is not null;
\ No newline at end of file
</file context>
| } catch (error) { | ||
| if (error instanceof Error && error.message === 'Redis rate-limit request timed out') { | ||
| const resettable = client as Redis & { disconnect?: () => void; connect?: () => void } | ||
| resettable.disconnect?.() |
There was a problem hiding this comment.
P1: When a rate-limit EVAL times out, this branch disconnects the process-wide Redis client, interrupting unrelated commands that share its socket and potentially leaving the public API's Redis-backed features unavailable during reconnect. Do not reset a shared client from the store; use a dedicated connection or add coordinated reset logic at the shared-client owner.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/services/rate-limit/stores/redis.ts, line 90:
<comment>When a rate-limit EVAL times out, this branch disconnects the process-wide Redis client, interrupting unrelated commands that share its socket and potentially leaving the public API's Redis-backed features unavailable during reconnect. Do not reset a shared client from the store; use a dedicated connection or add coordinated reset logic at the shared-client owner.</comment>
<file context>
@@ -75,13 +75,25 @@ export function createRedisStore(client: Redis): RateLimitStore {
+ } catch (error) {
+ if (error instanceof Error && error.message === 'Redis rate-limit request timed out') {
+ const resettable = client as Redis & { disconnect?: () => void; connect?: () => void }
+ resettable.disconnect?.()
+ resettable.connect?.()
+ }
</file context>
| }) | ||
| ) | ||
| } | ||
| if (linkedContact.email && !params.subscribedEmails?.has(linkedContact.email.trim().toLowerCase())) { |
There was a problem hiding this comment.
P2: When the subscriber email send fails, this condition still treats the address as delivered and suppresses the linked-contact fallback email. Track only successfully delivered subscriber addresses before applying this suppression, or retain the contact delivery when the subscriber send rejects.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/utils/feedback-support-notifications.ts, line 62:
<comment>When the subscriber email send fails, this condition still treats the address as delivered and suppresses the linked-contact fallback email. Track only successfully delivered subscriber addresses before applying this suppression, or retain the contact delivery when the subscriber send rejects.</comment>
<file context>
@@ -59,7 +59,7 @@ export async function notifyLinkedFeedbackContacts(params: {
)
}
- if (linkedContact.email) {
+ if (linkedContact.email && !params.subscribedEmails?.has(linkedContact.email.trim().toLowerCase())) {
deliveries.push(
sendStatusChangeNotificationEmail({
</file context>
| } | ||
| logger.info('Rate limit store: redis') | ||
| return createRedisStore( | ||
| createRedisConnection(url, 'rate-limit', { |
There was a problem hiding this comment.
P2: When realtime and rate limiting are enabled in the same process, this creates a second regular-command Redis socket instead of using the shared publisher connection. That defeats the documented connection-count guarantee and can exhaust Redis client limits as instances scale; keep the limiter on a cached shared command connection, or add a cached shared variant that preserves the fail-fast options.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/services/rate-limit/index.ts, line 38:
<comment>When realtime and rate limiting are enabled in the same process, this creates a second regular-command Redis socket instead of using the shared publisher connection. That defeats the documented connection-count guarantee and can exhaust Redis client limits as instances scale; keep the limiter on a cached shared command connection, or add a cached shared variant that preserves the fail-fast options.</comment>
<file context>
@@ -34,7 +34,12 @@ function createStore(): RateLimitStore {
logger.info('Rate limit store: redis')
- return createRedisStore(getSharedRedisClient(url))
+ return createRedisStore(
+ createRedisConnection(url, 'rate-limit', {
+ enableOfflineQueue: false,
+ maxRetriesPerRequest: 1,
</file context>
| expect(projectResponse.ok()).toBe(true) | ||
| const projectPayload = await projectResponse.json() | ||
| const originalSettings = projectPayload?.data?.settings ?? null | ||
| const roadmapSettings = { ...(originalSettings || {}), roadmapEnabled: true } |
There was a problem hiding this comment.
P2: When the seeded team has Roadmap disabled, this setup does not enable it because it updates project.settings instead of the team module configuration. The test then cannot find the Roadmap link; enable the team’s roadmap module through its modules API or seed the required team state in the test fixture.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/e2e/anonymous-feedback.spec.ts, line 266:
<comment>When the seeded team has Roadmap disabled, this setup does not enable it because it updates `project.settings` instead of the team module configuration. The test then cannot find the Roadmap link; enable the team’s roadmap module through its modules API or seed the required team state in the test fixture.</comment>
<file context>
@@ -254,25 +257,45 @@ test.describe('Anonymous feedback sessions', () => {
+ expect(projectResponse.ok()).toBe(true)
+ const projectPayload = await projectResponse.json()
+ const originalSettings = projectPayload?.data?.settings ?? null
+ const roadmapSettings = { ...(originalSettings || {}), roadmapEnabled: true }
+ const updateResponse = await request.put(`/api/projects/${PROJECT_SLUG}`, {
+ data: { settings: roadmapSettings },
</file context>
|
|
||
| /** Open the socket if it isn't already open/connecting. Safe to call repeatedly. */ | ||
| connect(): void { | ||
| this.intentionalClose = false |
There was a problem hiding this comment.
P2: When a tab becomes visible while an idle socket is still closing, connect() clears the idle close intent before the close callback runs. Keep idle-close state separate from the pagehide close intent so visible resumes reconnect without scheduling backoff.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/realtime-client.ts, line 186:
<comment>When a tab becomes visible while an idle socket is still closing, `connect()` clears the idle close intent before the close callback runs. Keep idle-close state separate from the pagehide close intent so visible resumes reconnect without scheduling backoff.</comment>
<file context>
@@ -182,6 +183,7 @@ export class RealtimeClient {
/** Open the socket if it isn't already open/connecting. Safe to call repeatedly. */
connect(): void {
+ this.intentionalClose = false
void this.open()
}
</file context>
| @@ -29,6 +29,20 @@ export class StaticCnameDomainProvider implements DomainProvider { | |||
|
|
|||
| async registerProjectDomain(input: { hostname: string }) { | |||
| const hostname = normalizeDomainHostname(input.hostname) | |||
| if (hostname === 'localhost' || hostname.endsWith('.localhost')) { | |||
There was a problem hiding this comment.
P2: The localhost shortcut only exists in registerProjectDomain, so a localhost custom domain flips back to dns_required on any re-check. getProjectDomainStatus (which verifyProjectDomain delegates to) performs dns.resolveCname, which cannot resolve .localhost per RFC 6761 and throws, so the catch returns buildPendingResult with verified=false and status='dns_required'. The verify flow (server/api/projects/[slug]/verify-domain.get.ts) then persists that result via buildDomainSettingsPatch/search persistProjectDomainResult, overwriting the active status and clearing domainVerifiedAt — undoing the localhost activation this change is meant to provide. Extract a shared localhost branch (e.g. buildLocalDevelopmentResult) and return it from getProjectDomainStatus before the DNS lookup as well.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/services/domains/providers/static-cname.ts, line 32:
<comment>The localhost shortcut only exists in registerProjectDomain, so a localhost custom domain flips back to dns_required on any re-check. getProjectDomainStatus (which verifyProjectDomain delegates to) performs dns.resolveCname, which cannot resolve .localhost per RFC 6761 and throws, so the catch returns buildPendingResult with verified=false and status='dns_required'. The verify flow (server/api/projects/[slug]/verify-domain.get.ts) then persists that result via buildDomainSettingsPatch/search persistProjectDomainResult, overwriting the active status and clearing domainVerifiedAt — undoing the localhost activation this change is meant to provide. Extract a shared localhost branch (e.g. buildLocalDevelopmentResult) and return it from getProjectDomainStatus before the DNS lookup as well.</comment>
<file context>
@@ -29,6 +29,20 @@ export class StaticCnameDomainProvider implements DomainProvider {
async registerProjectDomain(input: { hostname: string }) {
const hostname = normalizeDomainHostname(input.hostname)
+ if (hostname === 'localhost' || hostname.endsWith('.localhost')) {
+ return {
+ hostname,
</file context>
| to = reportingDayBounds(toDate, reportingTimezone).end | ||
| } catch (error) { | ||
| if (error instanceof RangeError) { | ||
| throw createError({ statusCode: 400, statusMessage: 'Invalid reporting date range' }) |
There was a problem hiding this comment.
P3: The new 400 omits the structured error envelope that the rest of the support routes use for validation errors. Neighboring routes (e.g. server/api/support/contacts/[id].put.ts:72, server/api/support/conversations/[id].patch.ts:90, server/api/support/canned-responses/[id].delete.ts) throw createError({ statusCode, statusMessage, data: createErrorResponse(ErrorCode.VALIDATION_ERROR, message) }), so clients can branch on data.error.code. This response only carries statusMessage, so those clients receive no parsable error code. Align it with the existing pattern.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/api/support/teams/[teamId]/csat-summary.get.ts, line 89:
<comment>The new 400 omits the structured error envelope that the rest of the support routes use for validation errors. Neighboring routes (e.g. `server/api/support/contacts/[id].put.ts:72`, `server/api/support/conversations/[id].patch.ts:90`, `server/api/support/canned-responses/[id].delete.ts`) throw `createError({ statusCode, statusMessage, data: createErrorResponse(ErrorCode.VALIDATION_ERROR, message) })`, so clients can branch on `data.error.code`. This response only carries `statusMessage`, so those clients receive no parsable error code. Align it with the existing pattern.</comment>
<file context>
@@ -75,12 +75,21 @@ export default defineEventHandler(async (event) => {
+ to = reportingDayBounds(toDate, reportingTimezone).end
+ } catch (error) {
+ if (error instanceof RangeError) {
+ throw createError({ statusCode: 400, statusMessage: 'Invalid reporting date range' })
+ }
+ throw error
</file context>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/auth.ts">
<violation number="1" location="lib/auth.ts:90">
P2: When `APP_DOMAIN=localhost`, public-board sign-in redirects directly to `foo.localhost` without the handoff token this host-only cookie configuration requires. The session cookie set on `localhost` is not sent to the board, so users appear logged out; make local public-subdomain redirects use the handoff flow as well.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| enabled: true, | ||
| // Browsers reject a Domain=.localhost cookie for sibling localhost hosts. | ||
| // Local public boards use the auth handoff flow instead of a shared cookie. | ||
| enabled: supportsCrossSubdomainCookies, |
There was a problem hiding this comment.
P2: When APP_DOMAIN=localhost, public-board sign-in redirects directly to foo.localhost without the handoff token this host-only cookie configuration requires. The session cookie set on localhost is not sent to the board, so users appear logged out; make local public-subdomain redirects use the handoff flow as well.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/auth.ts, line 90:
<comment>When `APP_DOMAIN=localhost`, public-board sign-in redirects directly to `foo.localhost` without the handoff token this host-only cookie configuration requires. The session cookie set on `localhost` is not sent to the board, so users appear logged out; make local public-subdomain redirects use the handoff flow as well.</comment>
<file context>
@@ -84,7 +85,9 @@ export const auth = betterAuth({
- enabled: true,
+ // Browsers reject a Domain=.localhost cookie for sibling localhost hosts.
+ // Local public boards use the auth handoff flow instead of a shared cookie.
+ enabled: supportsCrossSubdomainCookies,
domain: '.' + appDomain,
},
</file context>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="playwright.config.ts">
<violation number="1" location="playwright.config.ts:34">
P2: In CI the webServer now runs a full production build (`yarn openapi:generate && nuxt build`) before `yarn preview` starts, but `webServer.timeout: 120_000` covers the entire startup window. On a cold GitHub Actions runner a Nuxt build for this codebase (~24 pages, 172 server routes, 323 sources) can plausibly exceed 120 s, which would fail every e2e run with a `config.webServer` timeout — exactly the environment the new path targets. This also duplicates the separate CI `build` job, whose `.output` is not shared with the e2e job (it is gitignored, so the build always reruns). Move the build into the workflow as an explicit step before `yarn test:e2e:if-available` and let the webServer command be just `yarn preview`, or raise the CI webServer timeout well above the expected cold-build time.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| : { | ||
| command: `yarn dev --host localhost --port ${PORT}`, | ||
| command: process.env.CI | ||
| ? `yarn build && yarn preview --host localhost --port ${PORT}` |
There was a problem hiding this comment.
P2: In CI the webServer now runs a full production build (yarn openapi:generate && nuxt build) before yarn preview starts, but webServer.timeout: 120_000 covers the entire startup window. On a cold GitHub Actions runner a Nuxt build for this codebase (~24 pages, 172 server routes, 323 sources) can plausibly exceed 120 s, which would fail every e2e run with a config.webServer timeout — exactly the environment the new path targets. This also duplicates the separate CI build job, whose .output is not shared with the e2e job (it is gitignored, so the build always reruns). Move the build into the workflow as an explicit step before yarn test:e2e:if-available and let the webServer command be just yarn preview, or raise the CI webServer timeout well above the expected cold-build time.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At playwright.config.ts, line 34:
<comment>In CI the webServer now runs a full production build (`yarn openapi:generate && nuxt build`) before `yarn preview` starts, but `webServer.timeout: 120_000` covers the entire startup window. On a cold GitHub Actions runner a Nuxt build for this codebase (~24 pages, 172 server routes, 323 sources) can plausibly exceed 120 s, which would fail every e2e run with a `config.webServer` timeout — exactly the environment the new path targets. This also duplicates the separate CI `build` job, whose `.output` is not shared with the e2e job (it is gitignored, so the build always reruns). Move the build into the workflow as an explicit step before `yarn test:e2e:if-available` and let the webServer command be just `yarn preview`, or raise the CI webServer timeout well above the expected cold-build time.</comment>
<file context>
@@ -30,7 +30,9 @@ export default defineConfig({
: {
- command: `yarn dev --host localhost --port ${PORT}`,
+ command: process.env.CI
+ ? `yarn build && yarn preview --host localhost --port ${PORT}`
+ : `yarn dev --host localhost --port ${PORT}`,
url: `${baseURL}/login`,
</file context>
There was a problem hiding this comment.
3 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/e2e/auth-settings-teams.spec.ts">
<violation number="1" location="tests/e2e/auth-settings-teams.spec.ts:52">
P2: `toBeEnabled()`/`toBeVisible()` are satisfied by server-rendered markup before Vue hydrates, so the removed `__vueParentComponent` probes were replaced by waits that do not gate hydration. The workspace-name input is enabled in SSR HTML (`:disabled="isCreatingOrg"`), and the settings-tab buttons are SSR-rendered. On slow/cold starts the subsequent `type()`/`click()` run before event listeners attach and are silently dropped, making the slug assertion and `toHaveURL(/#profile/)` flaky. This is the exact scenario the product test guards with the retrying `toPass` block; give the other interactions the same treatment.</violation>
<violation number="2" location="tests/e2e/auth-settings-teams.spec.ts:111">
P2: The settings tabs test now waits only for the tab to be visible, which server-rendered buttons satisfy before hydration, then clicks once. If the click lands before Vue attaches `setActiveTab`, `window.history.replaceState` never runs and `toHaveURL(/#profile/)` fails. Retrying the click until the URL reflects it (as the product test does with `toPass`) removes the race; later tab clicks are safe because hydration is guaranteed by then.</violation>
</file>
<file name="tests/e2e/organization-settings.spec.ts">
<violation number="1" location="tests/e2e/organization-settings.spec.ts:89">
P2: The removed `waitForFunction` sync on `__vueParentComponent` explicitly waited for Vue hydration before the click. `toBeVisible()` resolves much earlier: in this Nuxt SSR app, `pages/settings/index.vue` renders the `organization` tab in the server HTML (it is not filtered by `hasOrganization`), so the element is visible before hydration attaches the `@click` listener. Playwright's click actionability checks do not verify that listeners are attached, so the click can be dispatched and silently dropped. The masks in this file: `page.goto('/settings#organization')` already sets the hash and `SettingsPage.mounted()` selects the tab from the hash, so a lost click does not fail the URL/content assertions here — but the tests no longer verify the tab click works and the race can surface as intermittent flake.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const tab = document.querySelector('[data-testid="settings-tab-profile"]') as any | ||
| return Boolean(tab?.__vueParentComponent) | ||
| }) | ||
| await expect(page.locator(selectors.settingsTabProfile)).toBeVisible({ timeout: 20_000 }) |
There was a problem hiding this comment.
P2: The settings tabs test now waits only for the tab to be visible, which server-rendered buttons satisfy before hydration, then clicks once. If the click lands before Vue attaches setActiveTab, window.history.replaceState never runs and toHaveURL(/#profile/) fails. Retrying the click until the URL reflects it (as the product test does with toPass) removes the race; later tab clicks are safe because hydration is guaranteed by then.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/e2e/auth-settings-teams.spec.ts, line 111:
<comment>The settings tabs test now waits only for the tab to be visible, which server-rendered buttons satisfy before hydration, then clicks once. If the click lands before Vue attaches `setActiveTab`, `window.history.replaceState` never runs and `toHaveURL(/#profile/)` fails. Retrying the click until the URL reflects it (as the product test does with `toPass`) removes the race; later tab clicks are safe because hydration is guaranteed by then.</comment>
<file context>
@@ -118,10 +108,7 @@ test('settings navigation tabs render expected sections', async ({ page }) => {
- const tab = document.querySelector('[data-testid="settings-tab-profile"]') as any
- return Boolean(tab?.__vueParentComponent)
- })
+ await expect(page.locator(selectors.settingsTabProfile)).toBeVisible({ timeout: 20_000 })
await page.locator(selectors.settingsTabProfile).click()
</file context>
| @@ -49,6 +49,7 @@ test('onboarding slug mirrors the full workspace name while typing', async ({ pa | |||
|
|
|||
| const workspaceNameInput = page.getByLabel('Workspace name') | |||
| const workspaceSlugInput = page.getByLabel('URL') | |||
| await expect(workspaceNameInput).toBeEnabled() | |||
There was a problem hiding this comment.
P2: toBeEnabled()/toBeVisible() are satisfied by server-rendered markup before Vue hydrates, so the removed __vueParentComponent probes were replaced by waits that do not gate hydration. The workspace-name input is enabled in SSR HTML (:disabled="isCreatingOrg"), and the settings-tab buttons are SSR-rendered. On slow/cold starts the subsequent type()/click() run before event listeners attach and are silently dropped, making the slug assertion and toHaveURL(/#profile/) flaky. This is the exact scenario the product test guards with the retrying toPass block; give the other interactions the same treatment.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/e2e/auth-settings-teams.spec.ts, line 52:
<comment>`toBeEnabled()`/`toBeVisible()` are satisfied by server-rendered markup before Vue hydrates, so the removed `__vueParentComponent` probes were replaced by waits that do not gate hydration. The workspace-name input is enabled in SSR HTML (`:disabled="isCreatingOrg"`), and the settings-tab buttons are SSR-rendered. On slow/cold starts the subsequent `type()`/`click()` run before event listeners attach and are silently dropped, making the slug assertion and `toHaveURL(/#profile/)` flaky. This is the exact scenario the product test guards with the retrying `toPass` block; give the other interactions the same treatment.</comment>
<file context>
@@ -46,13 +46,10 @@ test('onboarding slug mirrors the full workspace name while typing', async ({ pa
const workspaceNameInput = page.getByLabel('Workspace name')
const workspaceSlugInput = page.getByLabel('URL')
+ await expect(workspaceNameInput).toBeEnabled()
await workspaceNameInput.click()
</file context>
| const tab = document.querySelector('[data-testid="settings-tab-organization"]') as any | ||
| return Boolean(tab?.__vueParentComponent) | ||
| }) | ||
| await expect(page.locator(selectors.settingsTabOrganization)).toBeVisible({ timeout: 20_000 }) |
There was a problem hiding this comment.
P2: The removed waitForFunction sync on __vueParentComponent explicitly waited for Vue hydration before the click. toBeVisible() resolves much earlier: in this Nuxt SSR app, pages/settings/index.vue renders the organization tab in the server HTML (it is not filtered by hasOrganization), so the element is visible before hydration attaches the @click listener. Playwright's click actionability checks do not verify that listeners are attached, so the click can be dispatched and silently dropped. The masks in this file: page.goto('/settings#organization') already sets the hash and SettingsPage.mounted() selects the tab from the hash, so a lost click does not fail the URL/content assertions here — but the tests no longer verify the tab click works and the race can surface as intermittent flake.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/e2e/organization-settings.spec.ts, line 89:
<comment>The removed `waitForFunction` sync on `__vueParentComponent` explicitly waited for Vue hydration before the click. `toBeVisible()` resolves much earlier: in this Nuxt SSR app, `pages/settings/index.vue` renders the `organization` tab in the server HTML (it is not filtered by `hasOrganization`), so the element is visible before hydration attaches the `@click` listener. Playwright's click actionability checks do not verify that listeners are attached, so the click can be dispatched and silently dropped. The masks in this file: `page.goto('/settings#organization')` already sets the hash and `SettingsPage.mounted()` selects the tab from the hash, so a lost click does not fail the URL/content assertions here — but the tests no longer verify the tab click works and the race can surface as intermittent flake.</comment>
<file context>
@@ -86,10 +86,7 @@ test('owner can view, update, and delete organization from settings tab', async
- const tab = document.querySelector('[data-testid="settings-tab-organization"]') as any
- return Boolean(tab?.__vueParentComponent)
- })
+ await expect(page.locator(selectors.settingsTabOrganization)).toBeVisible({ timeout: 20_000 })
await page.locator(selectors.settingsTabOrganization).click()
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
yarn harness:verifyDATABASE_URLis unsetPLAYWRIGHT_FORCE=1is unsetLuna implementation and review gates passed for each Stage 09 slice.
Summary by cubic
Builds the support platform through the Stage 09 reporting foundations so reporting can rely on durable status history and timezone-correct daily metrics. Deploys now require Node.js 22.12+, Redis/Valkey, and an explicit
yarn db:migrate:deploy;yarn buildno longer runs migrations.New Features
Bug Fixes
Written for commit c594e5d. Summary will update on new commits.