Repository navigation
Conversation
A signed-in user could not review the notifications an account received: the app had one notification settings screen and no list of past notifications. Add a Notification history row under Notifications in the Preferences hub and a screen that lists the recorded notifications newest first. Each row is rendered on the device from the stored catalog key and its params in the app language, falls back to the stored copy when that key is gone, and opens the same destination a push tap opens, including the organization switch. The list filters by the recorded categories, pages with a retention bound, pulls to refresh, and covers loading, empty with the retention window, error and retry states. A device whose pushes are off still lists what arrived and offers a link to the notification settings. The rows are typed from `user.getNotificationHistory`, and that output type only resolves once the tRPC bundle can resolve the notifications package it names: without it in the bundle's dependencies every mobile consumer saw the item type as an untyped index signature.
iscekic
added
drain
|
bot: Device captures from the Android emulator against the local dev stack, for the notification history screen at head Each assertion below was read from a
|
… a11y gallery The Preferences hub gained a row, so the gallery that mounts that screen needs the row's icon in its icon mock.
The extension typecheck reports the second parameter of `sessionCardStaleAtSeconds` as unused, because the stale moment is derived from the card's own last activity and the caller's default window. A change to the tRPC package's dependencies now marks the extension affected, so that pre-existing error blocks the typecheck group; remove the parameter and its argument. Behaviour is identical: the quiet window still counts from the last activity the server saw.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe incremental change gives the chip row's Assumptions: Files Reviewed (2 files)
Previous Review Summaries (4 snapshots, latest commit d2ed97e)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit d2ed97e)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe refresh-key fixes are correct — Assumptions: Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files changed in this revision)
Fix these issues in Kilo Cloud Previous review (commit b6928da)Status: 3 Issues Found | Recommendation: Address before merge Executive SummaryPull-to-refresh on the notification history screen still matches no cached query, because the refresh prefix is a Assumptions: Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files changed in this revision)
Fix these issues in Kilo Cloud Previous review (commit e258d8e)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (90 files)
Notes: the previous WARNINGs on the 150-row cap, the footer's tab-bar clearance, the chip Fix these issues in Kilo Cloud Previous review (commit ac8b67b)Status: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (97 files)
Notes: no security issue found; the server query is scoped to the authenticated user and the row payload is parsed with the shared schema. The Reviewed by deepseek-flash-latest · Input: 52.7K · Output: 7.1K · Cached: 425.6K Review guidance: REVIEW.md from base branch |
…empty filter Review follow-ups on the notification history screen: - A refresh now collapses the retained pages and reloads from the newest row. Each retained page is a keyset window that only stays contiguous with the page above it while page one's newest row is unchanged; re-requesting the old windows would skip every row between the old and the new page-one tail. The rows the refresh started from stay on screen until page one delivers. - The page bound says so in the footer instead of ending the list silently, and rises to 10 pages. - A first load while offline settles into the error state and its retry instead of pausing in `pending` and holding the skeleton. - The chip carries `role="tab"`, which `Button` keeps (`role` outranks `accessibilityRole`). - The next-page retry footer keeps the tab-bar clearance. - An empty category says so, and the retention window stays on the unfiltered empty state it describes. Two new catalog keys, translated into every catalog.
FlashList takes `onRefresh`/`refreshing`; it has no `refreshControl` prop, so the pull gesture never reached the handler on the device. The element stays on the state scrollers, which do accept one.
A tRPC query key carries `{}` as its second element, so `queryKey()` as a
prefix matches no infinite entry: the reset emptied nothing and the refetch
requested nothing, which is why a pull fetched no page. Reset through
`pathKey()` and refetch the entry's own `infiniteQueryKey`, and read the
failure from that exact key — `getQueryState` answers only an exact one.
The mounted fixture now builds tRPC's real key shapes, so the refresh case
fails if either key is wrong again, and it covers the refresh-failure toast.
|
bot: Two more captures from the Android emulator at head
|
|
bot: The rows in this capture came from the notifications worker itself, not from fixtures. Three Fabricated fixtures only: the organization, repository and amounts are invented, and the account is an owned fake account on the local stack.
|
iscekic
added
merge:blocked:kilobot-not-ready
The chip row's preferences read kept the default network mode, so a first load without connectivity paused in pending and left its skeletons on screen for as long as the device stayed offline. Give the read the same mode the list uses, so it answers or fails into the full chip set.
iscekic
added
merge:blocked:kilobot-not-ready
|
bot: Device captures from the Android emulator at head
|
Evidence
Evidence: demonstrated — on the Android emulator against the local dev stack, the screen listed the account's recorded notifications with copy rendered from the stored catalog key and params, filtered them by a tapped category chip, opened the destination a push tap opens, showed the push-off banner with the list still loading, and stated the retention window when the account had no rows.
What the first run observed (head
43f31a2654), dev stack on this VM, owned fake account seeded into the local PostgreSQL, five rows written intouser_notification_historyin the exact column shape the notifications worker's recorder writes — categoriesbalanceAlertstwice,spendAlerts,securityFindings, and anagent_briefingrow):user.getNotificationHistoryover the local tRPC endpoint returned all five rows newest first, with the stored category, push type, catalog key and title.balanceAlertschip was absent on this account because the server marks that category unavailable without an organization. Tapping Spend alerts narrowed the list on the device to the single spend row; the mounted test pins the query input the chip produces.notificationPathForDataproduces for aspend_alertpush, without thevia=pushmarker.Screenshots attached below (this VM has no image input; each assertion above is quoted from a
uiautomatordump taken in the same command as its capture, and every PNG carries onlyIHDR/sRGB/sBIT/IDAT/IEND).Re-verified on head
d2ed97ef03(after the review follow-ups, with a restarted Metro bundle):The three newest rows in that run came from the notifications worker itself, not from a fixture: a local worker with a temporary config that omits the Secrets Store block (its secret then comes from
.dev.vars) accepted threePOST /internal/v1/dispatchcalls through the push sink and recorded one row each —low_balance→balanceAlerts,spend_alert→spendAlerts,security_finding→securityFindings— and a replay of the first idempotency key answeredduplicateand added no row. The config change and a two-line local-only relaxation of the internal-secret check were reverted before this body was written; the tree is clean at the head. The app then pulled and rendered those rows with their catalog copy and params.Re-verified on head
611d4a5fbb(the chip-row offline fix; Metro restarted with a cleared cache, owned fake account, four rows seeded into the local PostgreSQL in the worker's column shape):Limits of this proof: iOS was not exercised, because no iOS-only code or mobile line under
Platform.OS/Platform.selectchanged. Real Expo delivery is not part of it: the sink answers for an accepted send, so the rows prove recording, querying and rendering, not device delivery. Pagination past the first page was not driven on the device — the mounted suite covers the second page, the retry footer and the retention bound.Change
/(app)/(tabs)/(3_profile)/notification-history).notification-history-screen.tsx: the list. An infinite query overuser.getNotificationHistorybounded by the shared retention helper so a newest-first list refuses its next page instead of lettingmaxPagesdrop the newest one; pull to refresh; skeleton rows;QueryErrorwith retry for a failed first page; a retry footer for a failed next page; an empty state carrying the retention window.lib/notification-history.ts: the row contract. Copy is rendered on the device from the stored catalog key and params through the shared push catalog in the app language, falling back to the stored title and body when the key is gone (never the content-free preview copy, which is a per-device send-time setting and is not what was recorded). A row's destination isnotificationPathForData, with thevia=pushanalytics marker removed, and the stored payload parsed first: a payload this app cannot parse has no destination, shows a toast and stays on the list.maxPagesdrop the newest, and the footer states when the bound — not the server — ends the list.NOTIFICATION_HISTORY_CATEGORIESnames — the categories the recorder can actually produce — and drops a category the server marks unavailable for the account.use-notification-master-gate.ts: the settings screen's master-gate computation as a hook (OS permission plus a token registered on the account), so the banner and the master switch cannot disagree.notifications.history.*inen.json, translated into all 86 other catalogs; chip labels reuse the settings screen's existing category copy.sessionCardStaleAtSecondsloses its unusednowSecondsparameter. The extension typecheck reports it as an unused parameter, and it now runs because this change touches the tRPC package, so the pre-existing error blocked the typecheck group. Behaviour is identical: the quiet window still counts from the card's last activity.packages/trpcnow declares@kilocode/notifications. Its bundled mobile declarations already importNotificationHistoryCategoryfrom that package, and without the dependency the import is unresolvable from the bundle, so every mobile consumer sawuser.getNotificationHistoryitems as an untyped index signature. With it declared, the row type is the real wire type.Verification
pnpm --filter kilo-app run typecheck, oxlint over the changed files,check:classes,check:plurals,check:i18n(87 catalogs matchen.json),catalog-parityand the Notifications settings suite.lib/notification-history.test.ts(copy rendering and fallback, in-language catalog copy, destination mapping with the marker stripped, unparseable payload, chip set and gating) andnotification-history-screen.mounted.test.tsx(rendered rows and relative time, chip filtering, row tap through the deep-link slot, dead-target toast, empty state with the retention window, retry state, banner with the list loaded, no banner when push is enabled).pending, and the mounted suite pins it: a first load with the online manager offline and a failing preferences read renders every chip rather than its skeletons (the test fails on the previous head).Production prerequisites
None.
Proxy answers
The owner's proxy gave these answers in the owner's place.
NOTIFICATION_HISTORY_CATEGORIES= chatMessages, agentAttention, sessionStatus, balanceAlerts, spendAlerts, securityFindings. two of the settings keys have no recorded push kind, so a chip for either can only ever answer an empty page (and one of those two belongs to an excluded feature). Which chips should the filter row carry?Answer: The 6 recorded categories
Confidence: 0.85 · Precedent: Product scope: excluded features stay untouched (session 2f71af3a787e); Trust code over documents — session 269375d6e466: "only trust code"
balanceAlertsandsecurityFindingsare unavailable for some accounts (the server reportscapabilities[key].available === falsewhen the account has no organization / no security agent). What should the chip do then?Answer: Hide the chip
Confidence: 0.75 · Precedent: UX: keep similar elements consistent across surfaces — session a247f3c19b1e: thought row should match the tool call row
Answer: Full master gate
Confidence: 0.80 · Precedent: UX: keep similar elements consistent across surfaces — session a247f3c19b1e; fix the class — session 00eb2a1d3e4b: "fix the class"
Answer: Unparseable stored payload
Confidence: 0.60 · Precedent: Speed: choose the simplest design that reaches the goal — session a30dc8dc131a: "Simplest please"
notificationPathForDataappends?via=push, which marks the rendered screen'ssession_viewedevent as push-originated. A history row is not a push tap. Keep the marker?Answer: Strip it
Confidence: 0.50 · Precedent: Engineering: instrument telemetry that shows waste and makes issues easy to debug — session 55c9c80332a7: owner asked for telemetry that shows waste
Answer: Empty state only
Confidence: 0.80 · Precedent: Speed and least work: simplest design that reaches the goal (session a30dc8dc131a, "Simplest please"); the brief places the retention note in the empty state
Answer: Preferences hub only
Confidence: 0.80 · Precedent: Speed and least work: do the least work possible with no drop in quality (session 7f3470e0ba6f, "do the least amount of work possible"); the brief names one entry point