Repository navigation
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryIncremental review at HEAD Assumptions: Files Reviewed (1 changed since previous review)
Previous Review Summaries (9 snapshots, latest commit 76ce90f)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 76ce90f)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review at HEAD Assumptions: Files Reviewed (5 files)
Previous review (commit 012388a)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review at HEAD Files Reviewed (5 files)
The incremental diff could not be reconstructed against the previous SHA, so a full-PR review was performed; the remaining files were unchanged and previously verified. Previous review (commit 731abc7)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review at HEAD Files Reviewed (1 changed file)
Incremental diff vs. prior reviewed state is the two import lines above; the remaining PR files were unchanged and previously verified. Previous review (commit 8a1bf88)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review at HEAD Files Reviewed (5 files)
Previous review (commit 95d377b)Status: No Issues Found | Recommendation: Merge Executive SummaryFull review at commit Files Reviewed (5 files)
Previous review (commit 9743a82)Status: No Issues Found | Recommendation: Merge Executive SummaryFull re-review at commit Files Reviewed (5 files)
Previous review (commit ca2a618)Status: No Issues Found | Recommendation: Merge Executive SummaryRe-reviewed the full PR (5 files) at commit Files Reviewed (5 files)
Previous review (commit 6e67c74)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of commit Files Reviewed (3 files)
Previous review (commit 7ea2528)Status: No Issues Found | Recommendation: Merge Reviewed the new user-scoped Files Reviewed (5 files)
Reviewed by deepseek-flash-latest · Input: 38.1K · Output: 10K · Cached: 670.2K Review guidance: REVIEW.md from base branch |
95d377b to
8a1bf88
Compare
341a0b2 to
731abc7
Compare
The feedback dialog forgot every submission once it closed. Add a user-scoped `list` query to cloudAgentNextFeedback and render the caller's five most recent submissions under the form, so people can see they already reported something and what they said. Closes Kilo-Org#6595
Clear auto-close timer on dialog change and unmount. Add live regions for submit and history states. Show load and error states for history. Return ISO timestamps from list. Order by id on ties. Trim list fields to id, text, created_at. Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>
- Cap the dialog at 85vh with scroll so a full history list cannot clip the send button on short viewports. - Test that orphaned rows whose owner was deleted (`kilo_user_id IS NULL`) are never returned. - Apply oxfmt to the routes/helper touched by the previous commit.
- Keep the dialog header and close button fixed; scroll only the body (`flex max-h-[85vh] flex-col` + inner `overflow-y-auto`), matching the tall-dialog pattern used elsewhere. - Drop the nested history scroller now that the body scrolls. - Prefer cached history over a transient background-refetch error. - Make the history error copy say how to retry. - Assert the 20-row cap with 25 rows instead of an empty table.
731abc7 to
012388a
Compare
…dback-history # Conflicts: # apps/web/src/routers/cloud-agent-next-feedback-router.test.ts
maphew
left a comment
maphew
left a comment
There was a problem hiding this comment.
Sweep: branch is up to date and the review is clean. Note this overlaps with No.6773, which now carries the same Cloud Agent feedback history after app-builder was removed upstream; only one should merge.
…dback-history # Conflicts: # apps/web/src/components/cloud-agent-next/FeedbackDialog.tsx # apps/web/src/routers/cloud-agent-next-feedback-router.ts
gil-kilo
left a comment
gil-kilo
left a comment
There was a problem hiding this comment.
Reviewed the full diff against main. Server-side scoping (ctx.user.id), input bounds, and ISO normalization all look right. Four inline notes attached: one on the offset handling in the timestamp normalizer (the same bug exists on both sides of the boundary), one on the success view hiding the refreshed history, one on the duplicated normalizer, and one on the default-limit test not actually pinning the default.
| if (!value) return null; | ||
| const iso = value.includes('T') | ||
| ? value | ||
| : value.replace(' ', 'T').replace(/([+-]\d{2})$/, '$1:00'); |
There was a problem hiding this comment.
Offset normalization only covers a bare ±HH.
The rewrite only handles +00, but the comment above promises we accept the driver's text shape generally. Two shapes break:
2026-01-01T00:00:00+0530(compact offset, emitted by Postgres/several serializers) fails the[+-]\d{2}:?\d{2}$check only if the:is required — here it passes — but2026-01-01 00:00:00+0530is rewritten by the([+-]\d{2})$branch only when it ends in exactly two digits after the sign, so the no-Tcompact form survives tonew Date(), which V8 parses as local time; the instant shifts.2026-01-01 00:00:00+05is rewritten to+0:00— not a valid offset — andnew Date()returnsInvalid Date, silently dropping the timestamp.
Normalize every offset shape explicitly before parsing:
function toIsoTimestamp(value: string | null | undefined): string | null {
if (!value) return null;
const normalized = value
.replace(' ', 'T')
.replace(/([+-]\d{2})(\d{2})$/, '$1:$2')
.replace(/([+-]\d{2})$/, '$1:00');
if (!/(?:[zZ]|[+-]\d{2}:\d{2})$/.test(normalized)) return null;
const time = new Date(normalized).getTime();
return Number.isNaN(time) ? null : new Date(time).toISOString();
}The identical helper in apps/web/src/components/cloud-agent-next/feedback-history.ts:22-25 has the same two bugs — worth fixing both from one shared function (see the note on that file).
|
|
||
| <div className="space-y-4"> | ||
| <div className="min-h-0 flex-1 space-y-4 overflow-y-auto"> | ||
| {showSuccess ? ( |
There was a problem hiding this comment.
The refreshed history is never visible on success.
onSuccess sets showSuccess, which replaces the whole body — including the new history block — and then closes the dialog after 1200 ms. handleOpenChange already refetches on reopen because the query is enabled: isOpen, so the invalidateQueries(...) added in onSuccess cannot deliver the stated "a new entry appears immediately" behavior: by the time the history could re-render, it is unmounted, and by the time the dialog reopens the fetch happens regardless.
Either keep the form and history mounted and show the success message inline (e.g. above the send button) so the invalidated list actually renders the new entry:
{showSuccess && (
<div className="flex items-center gap-2" role="status" aria-live="polite">
<Check className="h-4 w-4 text-green-500" />
<span className="text-sm text-green-500">Thank you for your feedback!</span>
</div>
)}
<Textarea ... />or, if the full-screen success state is intentional, drop the invalidateQueries call so the code does not imply a behavior the UI does not provide.
| // rejected rather than parsed as local time. | ||
| const iso = value.includes('T') | ||
| ? value | ||
| : value.replace(' ', 'T').replace(/([+-]\d{2})$/, '$1:00'); |
There was a problem hiding this comment.
Duplicated normalization with a divergent regex.
This is a copy of toIsoTimestamp in apps/web/src/routers/cloud-agent-next-feedback-router.ts:27-34, but the accept-check differs (/[zZ]|[+-]\d{2}:?\d{2}$/ here vs /(?:[zZ]|[+-]\d{2}:?\d{2})$/ there). Any fix to the offset handling has to be made twice, and a shape accepted by one side can be rejected by the other.
Extract one parseDbTimestamp(value: string | null | undefined): Date | null next to the shared timestamp boundary and have both callers use it, e.g.:
export function parseDbTimestamp(value: string | null | undefined): Date | null {
if (!value) return null;
const normalized = value
.replace(' ', 'T')
.replace(/([+-]\d{2})(\d{2})$/, '$1:$2')
.replace(/([+-]\d{2})$/, '$1:00');
if (!/(?:[zZ]|[+-]\d{2}:\d{2})$/.test(normalized)) return null;
const date = new Date(normalized);
return Number.isNaN(date.getTime()) ? null : date;
}The router then does parseDbTimestamp(row.created_at)?.toISOString() ?? null and this module does formatDistance(parseDbTimestamp(value)!, now, { addSuffix: true }).
| const caller = await createCallerForUser(regularUser.id); | ||
| const result = await caller.cloudAgentNextFeedback.list(); | ||
|
|
||
| expect(result.map(row => row.feedback_text)).toEqual([ |
There was a problem hiding this comment.
This does not pin the default limit.
Six rows are inserted and five are asserted, so the test passes for any DEFAULT_FEEDBACK_HISTORY_LIMIT from 5 to 20. Raising the default to 10 keeps the suite green, and the separate cap test only pins the maximum. Assert the length explicitly:
expect(result).toHaveLength(5);
expect(result.map(row => row.feedback_text)).toEqual([
'submission 5',
'submission 4',
'submission 3',
'submission 2',
'submission 1',
]);or compare a default call against list({ limit: 5 }) for equality.
Summary
Closes #6595. The Cloud Agent feedback dialog resets to blank every time it opens, so there is no way to tell whether something was already reported or what was said. This adds a user-scoped history list.
cloudAgentNextFeedback.list: returns the caller's own submissions, newest first, default 5 / max 20, with a deterministic tie-break. Scoped tokilo_user_id; no cross-user or admin read surface.created_atis normalized to UTC ISO at the JSON boundary perpackages/db/AGENTS.md("Timestamp boundaries").FeedbackDialog: fetches only while open, shows "Your recent feedback" under the form with loading / error / empty handling and live-region announcements, and invalidates the query after a successful submit so a new entry appears immediately.flex max-h-[85vh] flex-col), matching the tall-dialog pattern used elsewhere, and the auto-close timer is cleared on close/unmount.feedback-history.ts: relative-time formatter (date-fns) with a deterministicnowfor tests.Class vs individual
This covers the Cloud Agent feedback surface. The App Builder feedback dialog (separate router, project-scoped) has the same gap; left as a follow-up rather than widening this PR.
Verification
cloudAgentNextFeedback.listtests: own-rows-only, newest-first, default limit, explicit limit, 25-rows→20 cap, min/max/float rejection, orphanedkilo_user_id IS NULLrows excluded, empty list, ISO output shape.feedback-historyunit tests.TZ=Asia/Kolkata(+05:30) to check timestamp handling.oxlintclean on changed files,oxfmt --list-differentclean,tsgo --noEmit(apps/web) passes.Review
Two adversarial review passes were run before finalizing. Accepted findings (dialog scroll pattern, cache-vs-error precedence, cap assertion, copy, orphaned-row coverage) are applied. One reported major — that
toIsoTimestampcorrupts non-UTC timestamps because the pg driver returns aDate— was disproven here: the repo's documented production shape (packages/db/AGENTS.md:62) is thetimestamptztext value, and the ISO assertion passes underTZ=Asia/Kolkata. The normalization follows the documented boundary guidance.Visual changes
The dialog gains a muted "Your recent feedback" section under the send button, each item clamped to two lines with a relative timestamp.