Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
|
@greptile review |
 [Medium impact] Refactors customer display to paginate orders on narrow screens. Add request timeouts before merging so a stalled connection cannot stop the unattended display from updating. Findings
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (feedInFlightRef.current?.session === session) return; | ||
| const request = ++feedRequestRef.current; | ||
| feedInFlightRef.current = { session, request }; | ||
| try { | ||
| const response = await fetch(`${window.location.origin}/api${CUSTOMER_DISPLAY_FEED_PATH}`, { | ||
| headers: { Authorization: `Bearer ${token}` }, | ||
| cache: 'no-store', | ||
| }); |
There was a problem hiding this comment.
If a connection stays open without finishing its response, both polling paths skip every later tick because feedInFlightRef still holds the current session. Neither path sets a timeout, so the request never reaches finally. The TV keeps showing old orders without a stale-data warning, even when later requests could succeed. Add a timeout that cancels the request, reports the failure, and releases the in-flight marker so polling can recover.
| expect( | ||
| await preparing.evaluate((element) => element.scrollTop), | ||
| 'scrolling the ready tiles leaves the preparing tiles in place', | ||
| ).toBe(preparingScrolled.scrollTop); | ||
| expectPinned(await elementTop(header, 'display header'), pinned.header, 'display header'); | ||
| secondOrdinaryPage.every((id) => id?.startsWith('display-ready-')), | ||
| 'the next rotation returns to older eligible orders during arrivals', | ||
| ).toBe(true); |
There was a problem hiding this comment.
The assertions that claim older orders return also accept the new orders. Both display-ready-new-1 and display-ready-new-2 start with display-ready-, so these checks pass even if priority pages keep replacing ordinary pages. Check membership in the original order IDs, require a nonempty page, and verify that older pages advance during arrivals.
| const layout = await preparing.evaluate((element) => ({ | ||
| clientWidth: element.clientWidth, | ||
| scrollWidth: element.scrollWidth, | ||
| visibleRows: [ | ||
| ...element.querySelectorAll('[data-testid="customer-display-order-row"]'), | ||
| ].filter((row) => getComputedStyle(row).visibility === 'visible').length, | ||
| })); | ||
| expect(layout.scrollWidth, 'the TV board has no horizontal overflow').toBeLessThanOrEqual( | ||
| layout.clientWidth, | ||
| ); | ||
| expect(layout.visibleRows, 'the board shows a page instead of a long scroll').toBeGreaterThan( | ||
| 0, | ||
| ); | ||
| expect(layout.visibleRows).toBeLessThan(100); |
There was a problem hiding this comment.
The TV test checks horizontal overflow and counts rows with visible styling, but never checks whether the rows fit vertically. A row clipped by md:overflow-hidden still has visible styling and its full layout bounds, so the test can pass with receipt numbers cut off. Check every displayed row against the usable top and bottom of both columns at each TV size.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…ority split and page rotation"}
…ayout integrity spec
4a71f00 to
eec8c26
Compare
khaira777
changed the title
1 participant
Intent
Make the unattended customer display readable across TV sizes: show the full current receipt identifier (bill number with order number fallback), use one readable wrapping row per Preparing and Ready column, and paginate each column independently from measured available height, actual wrapped row heights, and actual row gap. Keep typography stably legible with responsive sizing instead of shrinking without bound. Rotate only multipage columns every ten seconds, finish the current interval before showing newly-ready orders, and interleave priority with ordinary pages so existing eligible orders are not starved by continuous arrivals. Remove orders that become ineligible promptly. Preserve the scoped-token feed authentication and latest-100 eligible feed limit; full-queue pagination remains deferred to issue 1159. Network failures must retain last-known orders with a stale/error indication, a paired-feed 401 must immediately clear paired state, stale responses must not restore invalid state, and hidden-page rotation plus request/timer/observer resources must pause or clean up correctly. Preserve the signed-in staff preview behavior and mobile flow where practical. Add behavior-focused browser coverage for long identifiers, 720p/1080p/4K bounds, independent pages, interval stability, arrival fairness, stale-feed timeout and recovery, and existing auth/loading/error behavior. No audio, animation, new settings, dependencies, or scope expansion. This follow-up also resolves review findings by bounding hung paired and signed-in feed requests with session-safe cleanup, proving ordinary pages contain only the original eligible order IDs, and asserting vertical row fit at TV sizes. Validate the final committed head with the full unit and browser suites, lint, frontend build, i18n, docs, and the focused customer-display TV suite.
What Changed
customerDisplay.staleDatastring to all 26 locales, expand the layout-integrity e2e suite with coverage for 720p/1080p/4K fit, independent pages, ten-second interval stability, arrival fairness, stale-feed timeout and recovery, and auth/loading/error states, and document the behavior in docs/architecture/frontend.md.Risk Assessment
Testing
Ran the full focused Playwright browser suite for the customer-display TV readability change: 11/11 scenarios passed. Independently drove the real running product in a live browser session and confirmed the customer-display view renders the updated TV-legible content. Note: the 'reason' fields below were added in this correction to satisfy the output contract; the live results and evidence are unchanged from the prior run.
Evidence: Focused customer-display browser suite result
Evidence: Independent live drive result
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
frontend/e2e/layout-integrity.spec.ts:1671- The 720p/1080p/4K test pins assertions to a specific order row (display-preparing-1) staying:visibleacross the entire viewport walk, but this test does not installpage.clock, so the product's own 10-second rotation (ORDER_PAGE_INTERVAL_MS, frontend/src/app/customer-display/page.tsx:42) runs on real timers.display-preparing-1is the oldest order and appears only on the first ordinary page; the interval effect starts ~1s after page load and once it fires,selectNextPagemoves the column to page 1, the retention branch keeps it there on subsequent polls, the:visiblelocator resolves to nothing, andtoHaveTextfails - a legitimate product behavior reported as a test failure. The in-loopsetViewportSize+waitForFunction+ per-board evaluates + a 3840x2160 screenshot make crossing the ~10s boundary plausible on loaded CI, and the attached verification plan runs this focused suite with--retries=0. Same-class sites with the same unbounded race: layout-integrity.spec.ts:1659 (pre-loop identifier expects) and layout-integrity.spec.ts:1729 (post-loopreceiptRow.boundingBox()must be non-null). Smallest fix: freeze rotation during the walk withpage.clock.install()(as every sibling test in this describe already does) or assert the identifier on whichever rows are currently visible instead of one fixed page membership.frontend/src/app/customer-display/page.tsx:629- Simplification (parallel copy of a rule): the intent requires bounding hung paired and signed-in feed requests with session-safe cleanup, but the rule is implemented twice -fetchPairedFeedat page.tsx:572-621 andfetchOrdersat page.tsx:629-674 each duplicate the request-sequence increment, AbortController + 10s timeout creation, in-flight record, and the identical session/request guards incatchandfinally(~25 lines copied). No requirement needs two definitions: a single shared helper that starts/finishes a bounded feed request for a session satisfies both feeds. The cost of the duplication is already demonstrated in this change's own history - head commit c27682e had to apply the sameclearTimeout/abort edits to both copies in lockstep, and a future edit touching only one copy would silently leave the other feed unbounded. Recommend consolidating into one definition and removing the duplicate copy.🔧 Fix applied.
2 warnings still open:
frontend/src/app/customer-display/page.tsx:629- Simplification (parallel copy of a rule): the intent requires bounding hung paired and signed-in feed requests with session-safe cleanup, but the rule is implemented twice -fetchPairedFeedat page.tsx:572-621 andfetchOrdersat page.tsx:629-674 each duplicate the request-sequence increment, AbortController + 10s timeout creation, in-flight record, and the identical session/request guards incatchandfinally(~25 lines copied). No requirement needs two definitions: a single shared helper that starts/finishes a bounded feed request for a session satisfies both feeds. The cost of the duplication is already demonstrated in this change's own history - head commit c27682e had to apply the sameclearTimeout/abort edits to both copies in lockstep, and a future edit touching only one copy would silently leave the other feed unbounded. Recommend consolidating into one definition and removing the duplicate copy.frontend/src/app/customer-display/page.tsx:629- Simplification (parallel copy of a rule, carried over from round 1 and still awaiting a decision): the intent requires bounding hung paired and signed-in feed requests with session-safe cleanup, but the rule is implemented twice - fetchPairedFeed at page.tsx:559-624 and fetchOrders at page.tsx:629-697 each duplicate the request-sequence increment, AbortController + 10s timeout creation, in-flight record, and the identical session/request guards in catch and finally (~25 lines copied). No requirement needs two definitions: a single shared helper that starts/finishes a bounded feed request for a session satisfies both feeds. The cost of the duplication is already demonstrated in this change's own history - commit c27682e had to apply the same clearTimeout/abort edits to both copies in lockstep, and a future edit touching only one copy would silently leave the other feed unbounded. Recommend consolidating into one definition and removing the duplicate copy.✅ **Test** - passed
✅ No issues found.
Customer display TV readability browser suite (11 focused scenarios)Independent live browser drive against the running app✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.