fix: ui improvements#377
Conversation
📝 WalkthroughWalkthroughThis pull request consolidates date formatting across the platform by introducing and migrating to a unified Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
services/platform/app/features/chat/components/chat-history-sidebar.tsx (1)
199-214:⚠️ Potential issue | 🟡 MinorNative input swap looks good; watch out for the Escape → blur race.
The switch to a native
<input>with ring-based focus indication is clean and maintains accessibility.However, there's a latent issue in the interaction between
handleCancelRename(called on Escape) andhandleInputBlur: when the user presses Escape,handleCancelRenamesetseditValueto''andeditingChatIdtonull. React then re-renders and unmounts the input, which fires theonBlurhandler from the previous render's closure — whereeditingChatIdstill equalschatId. This causeshandleSaveRenameto run with an emptyeditValue, showing a spurious "title empty" error toast.A simple guard would be to track cancellation intent so
handleInputBlurcan skip the save:Suggested fix
One approach — use a ref to signal cancellation:
+ const cancelledRef = useRef(false); + const handleCancelRename = () => { + cancelledRef.current = true; setEditingChatId(null); setEditValue(''); }; const handleInputBlur = (chatId: string) => { - if (editingChatId === chatId) { + if (editingChatId === chatId && !cancelledRef.current) { handleSaveRename(chatId); } + cancelledRef.current = false; };This is pre-existing, but since you're already touching this input element it would be a low-effort fix.
services/platform/lib/utils/date/format.ts (1)
56-71: 🧹 Nitpick | 🔵 TrivialConsider extracting the repeated date-parsing + validation boilerplate.
formatDate,formatDateSmart, andformatDateHeadereach contain a near-identical block: check string → append'Z'if needed →dayjs()→isValid()guard. A small internal helper would reduce duplication and keep future fixes (e.g., additional input normalization) in one place.♻️ Sketch
+function parseDateInput(date: string | Date | Dayjs): Dayjs | null { + let d: Dayjs; + if (typeof date === 'string') { + const utc = hasTimezoneInfo(date) ? date : date + 'Z'; + d = dayjs(utc); + } else { + d = dayjs(date); + } + return d.isValid() ? d : null; +}Then each public function simply calls
parseDateInputand handles thenullcase.Also applies to: 131-141, 177-187
services/platform/app/features/tone-of-voice/components/example-messages-table.tsx (1)
131-132: 🧹 Nitpick | 🔵 Trivial
formatDateis used in columns but missing from the dependency array.The
formatDatefunction fromuseFormatDate()is used on line 92 but omitted from theuseMemodeps. This is currently safe because theYYYY-MM-DDcustom format is locale-invariant, as the eslint-disable comment acknowledges. However, if this format is later changed to use a locale-sensitive preset, the stale closure would silently produce incorrect dates.Consider adding
formatDateto the dependency array for resilience, or documenting the locale-independence assumption more explicitly.♻️ Proposed fix
- // eslint-disable-next-line react-hooks/exhaustive-deps -- locale is not used in column definitions - [tTables, tCommon, onViewExample, onEditExample, onDeleteExample], + [tTables, tCommon, onViewExample, onEditExample, onDeleteExample, formatDate],services/platform/app/hooks/use-format-date.ts (1)
47-94: 🧹 Nitpick | 🔵 TrivialConsider memoizing the returned functions to stabilize references.
The four formatting functions (
formatDateWithLocale,formatDateSmartWithLocale,formatDateHeaderWithLocale,formatRelative) are recreated on every render. If consumers include them in dependency arrays (e.g.,useMemo,useEffect), each render would trigger re-computation. Wrapping them inuseCallback(with[locale, dateTranslations]as deps) would stabilize their identity across renders where locale hasn't changed.This isn't urgent — the current pattern works and the closures are cheap — but it would make the hook safer for consumers who do depend on function identity.
services/platform/app/features/chat/components/chat-search-dialog.tsx (1)
152-153:⚠️ Potential issue | 🟡 MinorDead code:
|| ''is unreachable.
new Date(chat.createdAt)always returns a (truthy) Date object, so|| ''never executes. The outerchat.createdAt &&guard already handles the falsy case. Remove|| ''to avoid confusion.🧹 Suggested fix
- {chat.createdAt && - formatDateSmart(new Date(chat.createdAt) || '')} + {chat.createdAt && + formatDateSmart(new Date(chat.createdAt))}services/platform/app/features/chat/components/chat-input.tsx (1)
196-207: 🧹 Nitpick | 🔵 TrivialConsider extracting the file-type label logic into a helper.
The ternary chain is now 5 levels deep and becoming hard to follow. A small helper function (e.g.,
getFileTypeLabel(attachment): string) would improve readability and make it easier to extend with new file types.♻️ Suggested approach
+function getFileTypeLabel( + attachment: FileAttachment, + tChat: (key: string) => string, +): string { + if (attachment.fileType === 'application/pdf') return tChat('fileTypes.pdf'); + if (attachment.fileType.includes('word')) return tChat('fileTypes.doc'); + if (attachment.fileType.includes('presentation') || attachment.fileType.includes('powerpoint')) + return tChat('fileTypes.pptx'); + if (attachment.fileType === 'text/plain') return tChat('fileTypes.txt'); + if (isTextBasedFile(attachment.fileName, attachment.fileType)) + return getFileExtensionLower(attachment.fileName).toUpperCase() || tChat('fileTypes.txt'); + return tChat('fileTypes.file'); +}services/platform/app/hooks/__tests__/use-infinite-scroll.test.ts (1)
262-276: 🧹 Nitpick | 🔵 TrivialNo-op assertion — this test verifies nothing.
Line 274 (
expect(true).toBe(true)) is a tautological assertion that always passes regardless of the threshold value. The newer tests at lines 307-330 properly verify threshold behavior viarootMargin. Consider removing this test or replacing the assertion with a meaningful one.♻️ Proposed fix: verify rootMargin includes the threshold
it('uses provided threshold value', () => { const onLoadMore = vi.fn(); - renderHook(() => - useInfiniteScroll({ + render( + React.createElement(TestComponent, { onLoadMore, hasMore: true, isLoading: false, threshold: 700, - }) + }) ); - expect(true).toBe(true); + expect(observerOptions?.rootMargin).toBe('0px 0px 700px 0px'); });services/platform/app/features/documents/components/onedrive-import-dialog.tsx (1)
247-259:⚠️ Potential issue | 🟠 MajorRemove unused
selectedItemsprop from interface and callers.
OneDriveFileTablePropsdeclaresselectedItems(line 252) and callers pass it (lines 1023, 1160), but the function creates its own local state instead of using the prop. The unused prop should be removed from the interface definition to avoid confusion.services/platform/app/features/documents/components/documents-client.tsx (1)
301-331:⚠️ Potential issue | 🟡 MinorEdge case: teams column shows nothing (no dash) when all tag IDs are unresolved.
If
tagsis non-empty but none of the IDs exist inteamMap(e.g., stale tag references), every.map()returnsnulland the rendered output is an emptyHStack— no badges and no "—" fallback. The user sees a blank cell instead of a meaningful placeholder.Proposed fix
return ( <HStack gap={1} className="flex-wrap"> - {tags.slice(0, 2).map((tagId) => { + {tags.slice(0, 2).flatMap((tagId) => { const teamName = teamMap.get(tagId); - if (!teamName) return null; + if (!teamName) return []; return ( - <Badge key={tagId} variant="blue" className="text-xs"> + [<Badge key={tagId} variant="blue" className="text-xs"> {teamName} - </Badge> + </Badge>] ); })} + {tags.slice(0, 2).every((tagId) => !teamMap.get(tagId)) && ( + <span className="text-muted-foreground text-sm">—</span> + )} {tags.length > 2 && (Alternatively, filter first and check if the resolved list is empty:
cell: ({ row }) => { const tags = row.original.teamTags; if (row.original.type === 'folder' || !tags || tags.length === 0) { return <span className="text-muted-foreground text-sm">—</span>; } if (isLoadingTeams) { return <Skeleton className="h-5 w-20" />; } + const resolvedTags = tags.filter((tagId) => teamMap.has(tagId)); + if (resolvedTags.length === 0) { + return <span className="text-muted-foreground text-sm">—</span>; + } return ( <HStack gap={1} className="flex-wrap"> - {tags.slice(0, 2).map((tagId) => { - const teamName = teamMap.get(tagId); - if (!teamName) return null; - return ( - <Badge key={tagId} variant="blue" className="text-xs"> - {teamName} - </Badge> - ); - })} - {tags.length > 2 && ( + {resolvedTags.slice(0, 2).map((tagId) => ( + <Badge key={tagId} variant="blue" className="text-xs"> + {teamMap.get(tagId)} + </Badge> + ))} + {resolvedTags.length > 2 && ( <Badge variant="outline" className="text-xs"> - +{tags.length - 2} + +{resolvedTags.length - 2} </Badge> )} </HStack>
🤖 Fix all issues with AI agents
In
`@services/platform/app/features/approvals/components/approval-detail-dialog.tsx`:
- Line 180: The code is converting approvalDetail.createdAt to an ISO string
before formatting, which is redundant and inconsistent with other usages; update
the call in approval-detail-dialog's render where formatDate is used so it
receives a Date object directly (i.e., pass new Date(approvalDetail.createdAt)
to formatDate instead of new Date(...).toISOString()), keeping the same 'long'
format argument and leaving the formatDate function and approvalDetail reference
unchanged.
In `@services/platform/app/features/approvals/components/approvals-client.tsx`:
- Line 725: The call to formatDate currently converts the Date to a string via
.toISOString(); update the expression that renders reviewedAt in
approvals-client.tsx so it passes a Date object directly to formatDate (remove
the .toISOString() call) — locate the usage around formatDate(new
Date(row.original.reviewedAt).toISOString(), 'short') and change it to pass new
Date(row.original.reviewedAt) with the same 'short' format argument so it
matches other files like api-key-table.tsx and audit-log-table.tsx.
In `@services/platform/app/features/chat/components/chat-input.tsx`:
- Line 138: The file input's hard-coded accept attribute in ChatInput
(chat-input.tsx) is missing many extensions used by
isTextBasedFile/TEXT_FILE_EXTENSIONS, causing valid text files to be hidden in
the native picker; update ChatInput to derive the accept string from
TEXT_FILE_EXTENSIONS (or expand the accept list to include the missing entries
like .rst, .tex, .scss, .sass, .less, .mjs, .cjs, .pyi, .toml, .ini, .cfg,
.conf, .env, etc.), e.g., import or reference TEXT_FILE_EXTENSIONS and join them
into a comma-separated string for the input accept prop so the picker shows all
supported text file types.
In `@services/platform/app/features/chat/components/message-bubble.tsx`:
- Around line 247-249: Extract the duplicated ternary/file-type logic into a
shared helper function (e.g., getFileTypeLabel(fileName, fileType, t)) and
replace the inline logic in both FileAttachmentDisplay and FilePartDisplay with
calls to that helper; the helper should use the existing utilities
isTextBasedFile and getFileExtensionLower and fallback to t('fileTypes.txt') or
t('fileTypes.file') as currently done, and update both places that render the
label (the expressions using isTextBasedFile, getFileExtensionLower, and t) to
call getFileTypeLabel(fileName, fileType, t) to eliminate duplication and keep
behavior consistent.
In `@services/platform/app/features/chat/hooks/use-convex-file-upload.ts`:
- Around line 57-61: The Content-Type header must fallback when file.type is
empty: in use-convex-file-upload (where you validate with mergedConfig and
isTextBasedFile) update the code that sets the 'Content-Type' header for the
upload to use file.type || 'application/octet-stream' (same pattern used in
use-document-upload and convex/http). Locate the upload/request builder that
uses file.type and replace it with the fallback so text-based files with empty
MIME types still get a safe Content-Type.
In
`@services/platform/app/features/documents/components/document-preview-text.tsx`:
- Around line 46-49: The isCodeFile check currently excludes the 'data' category
so JSON/YAML/XML render as plain text; update the isCodeFile determination (the
constant named isCodeFile that uses getTextFileCategory(fileName || '')) to
include 'data' (i.e., treat category === 'data' as code) so Shiki can apply
syntax highlighting for data files, or if exclusion was intentional add a brief
comment next to isCodeFile explaining why 'data' is omitted.
- Around line 9-31: The current decodeWithEncoding loop includes 'iso-8859-1' in
SUPPORTED_ENCODINGS which never throws or produces U+FFFD, so the loop will
always return on that entry and the UTF‑8 fallback is unreachable; either remove
'iso-8859-1' from SUPPORTED_ENCODINGS and keep it only as an explicit
last-resort decode after the loop, or keep the array but skip 'iso-8859-1'
inside decodeWithEncoding and handle it only after attempting the other
encodings (or add a comment documenting that iso-8859-1 is intentionally treated
as final fallback). Update the SUPPORTED_ENCODINGS/loop logic in
decodeWithEncoding accordingly so the fallback TextDecoder('utf-8', { fatal:
false }) can actually run.
- Around line 74-83: The effect that calls highlightCode (triggered by content,
ext, isCodeFile, shikiTheme) must reset the stale highlightedHtml immediately
when inputs change: at the top of the useEffect for document-preview-text, call
setHighlightedHtml('') (or null) before starting the async highlight so the
previous file/theme HTML isn't shown while highlightCode(resolveLanguage(ext),
shikiTheme) runs; keep the existing cancelled flag and only
setHighlightedHtml(html) when not cancelled and html is truthy, but ensure the
reset happens unconditionally when content/isCodeFile/ext/shikiTheme change.
- Around line 97-100: The highlightedHtml string is being inserted via
dangerouslySetInnerHTML without a sanitization layer; wrap or replace
highlightedHtml with a sanitized version before passing it to
dangerouslySetInnerHTML (e.g., import and use DOMPurify.sanitize or an
equivalent sanitizer) in the DocumentPreviewText component so the value passed
to dangerouslySetInnerHTML={{ __html: ... }} is sanitized; reference the
highlightedHtml variable and the component/file document-preview-text.tsx when
making the change.
- Around line 51-72: The effect in the DocumentPreviewText component uses a
local cancelled flag but does not abort the in-flight fetch; update the
useEffect's load to create an AbortController, pass controller.signal into
fetch(url, { signal }), and call controller.abort() in the cleanup to stop the
request when unmounting or url changes; inside the catch, detect an abort (e.g.,
DOMException or error.name === 'AbortError') and avoid setting error/state for
aborted requests, while still using decodeWithEncoding, setContent, setError and
setLoading as before for real errors.
In `@services/platform/app/features/documents/components/document-preview.tsx`:
- Around line 102-104: The text-file detection in isTextBasedFile fails when url
contains query params (e.g. file.txt?token=...), because it uses
getFileExtensionLower which slices raw strings; update isTextBasedFile to use
getFileExtension (the same URL-aware parser used earlier at line 49) when
fileName is falsy so query parameters are stripped, and also normalize the
fallback KNOWN_TEXT_FILENAMES check to compare against the parsed pathname (use
getFileExtension or URL-parsed pathname) so DocumentPreviewText routing remains
consistent with the earlier getFileExtension(fileName || url) behavior.
In
`@services/platform/app/features/settings/audit-logs/components/audit-log-table.tsx`:
- Line 96: The useMemo for the column definitions is listing formatDate in its
dependency array even though only t and formatRelative are used inside; update
the dependency array for the memoized columns to remove formatDate so it only
depends on [t, formatRelative], keeping the memoization correct for the column
definitions (refer to the useMemo that returns the column definitions and the
symbols t, formatRelative, and formatDate).
In
`@services/platform/app/features/tone-of-voice/components/example-messages-table.tsx`:
- Around line 90-93: Replace the explicit undefined preset in the cell renderer
call to formatDate so intent is clear: in the cell for row.original.updatedAt
inside example-messages-table.tsx, change formatDate(row.original.updatedAt,
undefined, { customFormat: 'YYYY-MM-DD' }) to pass a named preset (for example
'default' or 'iso') or remove the second argument if the API supports an
overload; update the call in the cell: ({ row }) => (...) so the preset is
explicit (e.g., formatDate(row.original.updatedAt, 'default', { customFormat:
'YYYY-MM-DD' })).
In `@services/platform/app/routes/dashboard/`$id/settings/logs.tsx:
- Around line 17-53: The LogsSkeleton component shows a pagination placeholder
that the real AuditLogTable doesn't use; update the DataTableSkeleton call
inside LogsSkeleton to pass showPagination={false} so the skeleton matches the
actual table layout (locate the DataTableSkeleton invocation in the LogsSkeleton
function and add the prop).
In `@services/platform/lib/utils/date/format.ts`:
- Around line 22-28: The applyLocale function can silently fall back to Dayjs's
global/default locale when the requested locale or its base aren't registered
and also mishandles an empty-string input; update applyLocale to guard against
falsy/empty locale inputs (return the original Dayjs object or no-op) before
calling locale(), compute key and base as now, attempt the regional locale
(withRegion) and only return it if its .locale() matches key, otherwise if key
!== base attempt the base locale and return that if registered, and finally
return the original Dayjs object (or explicit no-op) to avoid silently relying
on Dayjs's default; also add a JSDoc on applyLocale explaining that when neither
regional nor base locale is registered Dayjs will fall back to its
global/default locale (commonly 'en') so maintainers are aware.
In `@services/platform/lib/utils/shiki.ts`:
- Around line 9-19: The cached promise highlighterPromise is never cleared if
createHighlighter rejects, causing permanent failure; update getHighlighter to
attach a .catch handler (or use try/catch with async/await) to reset
highlighterPromise to null on rejection so subsequent calls retry, ensuring you
still return the rejection to callers while clearing the cache; locate the
highlighterPromise and getHighlighter function and implement the rejection-reset
logic around the createHighlighter(...) invocation.
- Around line 52-69: The catch block in highlightCode currently returns an empty
string when hl.loadLanguage(resolvedLang) fails, which hides code blocks;
instead, update highlightCode (function name highlightCode, using
getHighlighter(), resolveLanguage(), hl.loadLanguage(), and hl.codeToHtml()) so
that on a loadLanguage error you fall back to Shiki's plaintext language by
returning hl.codeToHtml(code, { lang: 'text', theme }) (or equivalent) rather
than returning '', ensuring code remains visible; keep the try/catch around
hl.loadLanguage and only substitute 'text' in the catch branch.
In `@services/platform/lib/utils/text-file-types.ts`:
- Around line 7-35: Define the category sets (CODE_EXTENSIONS,
CONFIG_EXTENSIONS, MARKUP_EXTENSIONS, DATA_EXTENSIONS, and a small
TEXT_EXTENSIONS for plain text like 'txt','log') first, then build
TEXT_FILE_EXTENSIONS as the union of those sets instead of hard-coding
duplicates; update the file so functions/constants referencing
TEXT_FILE_EXTENSIONS use the computed union and remove repeated entries from
TEXT_FILE_EXTENSIONS, ensuring any new extension is added to the appropriate
category set and automatically included in TEXT_FILE_EXTENSIONS.
In `@services/platform/package.json`:
- Line 108: Change the dependency version from "shiki": "^3.22.0" to "shiki":
"^3.21.0" in package.json, and update any code that imports the full package to
use the lighter-weight API: replace imports from "shiki" with "shiki/core" and
switch to the JS regex engine usage (e.g., use
createHighlighter/loadTheme/loadLanguage or registerLanguage APIs) so you only
import the specific languages and themes required; ensure package.json and any
modules referencing the old import are updated accordingly to avoid bundling the
full grammar/theme set.
In `@services/rag/app/routers/documents.py`:
- Around line 232-258: SUPPORTED_EXTENSIONS currently includes risky types (.env
and .log); remove those extensions from SUPPORTED_EXTENSIONS or add
pre-ingestion scanning in the upload/ingestion path (e.g., implement a
scan_file_for_secrets(file_bytes) helper and call it from the document
upload/ingest handler such as ingest_document or upload_document_handler before
indexing). The scanner should check for credential patterns (API_KEY=,
PASSWORD=, SECRET=, Bearer\s+[A-Za-z0-9\-_\.]+, connection strings, key-like
hex/base64 tokens) and return a reject flag and reason; if rejected, do not
index and return a 4xx error to the caller. Ensure tests or logs clearly show
files rejected for secrets and that SUPPORTED_EXTENSIONS no longer lists ".env"
and ".log" if you choose the removal option.
- Around line 232-258: The backend's SUPPORTED_EXTENSIONS set (symbol:
SUPPORTED_EXTENSIONS) drifts from the frontend's hardcoded upload MIME/extension
list, causing UX failures; fix by extracting the canonical extension list into a
shared module (e.g., a single exported constant file) that both backend routers
(where SUPPORTED_EXTENSIONS is used) and the frontend upload component import,
updating references to use that constant, and add a unit/integration test that
asserts the frontend's accepted extensions equal the shared constant to prevent
future drift.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Improvements