Preset text pair subtask#232
Conversation
Confidence Score: 4/5The new preset functionality is additive and well-scoped, but auth handling in presetApi.ts and version display in edit mode have concerns flagged in prior review threads that remain unresolved. The core flow (select language → select version → save preset → display in view) is implemented correctly. The previously noted issues in presetApi.ts around auth header construction and the truncated-UUID display in SubTaskCard remain open, and these affect user-facing correctness on every preset interaction. src/components/routes/task/api/presetApi.ts and src/components/ui/molecules/subtask-card/SubTaskCard.tsx still carry concerns from prior review threads that should be addressed before merging. Reviews (2): Last reviewed commit: "format update" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const getAuthHeaders = () => ({ | ||
| Authorization: `Bearer ${sessionStorage.getItem("accessToken")}`, | ||
| }); | ||
|
|
||
| export interface PresetRequest { |
There was a problem hiding this comment.
getAuthHeaders() helper is redundant and dangerous. The axiosInstance already has a request interceptor in axios-config.ts that reads sessionStorage.getItem(ACCESS_TOKEN) with a null guard (if (token) { config.headers.Authorization = ... }). By passing per-request headers here, when sessionStorage.getItem("accessToken") returns null (expired session, fresh tab, etc.) the literal string "Bearer null" is forwarded — the interceptor's null check won't fire because it only sets the header when the token exists but doesn't clear a pre-set header. Every other API file (e.g. searchApi.ts) relies solely on the interceptor and is unaffected by this problem. The getAuthHeaders calls should be removed from all three functions.
| const getAuthHeaders = () => ({ | |
| Authorization: `Bearer ${sessionStorage.getItem("accessToken")}`, | |
| }); | |
| export interface PresetRequest { | |
| export interface PresetRequest { |
| const { data } = await axiosInstance.post( | ||
| `/api/v1/cms/sub-tasks/${subtaskId}/preset`, | ||
| preset, | ||
| { | ||
| headers: getAuthHeaders(), | ||
| } | ||
| ); |
There was a problem hiding this comment.
headers option from createOrUpdatePreset (the interceptor already handles auth).
| const { data } = await axiosInstance.post( | |
| `/api/v1/cms/sub-tasks/${subtaskId}/preset`, | |
| preset, | |
| { | |
| headers: getAuthHeaders(), | |
| } | |
| ); | |
| const { data } = await axiosInstance.post( | |
| `/api/v1/cms/sub-tasks/${subtaskId}/preset`, | |
| preset | |
| ); |
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!
| const { data } = await axiosInstance.get( | ||
| `/api/v1/cms/sub-tasks/${subtaskId}/preset`, | ||
| { | ||
| headers: getAuthHeaders(), | ||
| } | ||
| ); |
There was a problem hiding this comment.
headers option from getPreset.
| const { data } = await axiosInstance.get( | |
| `/api/v1/cms/sub-tasks/${subtaskId}/preset`, | |
| { | |
| headers: getAuthHeaders(), | |
| } | |
| ); | |
| const { data } = await axiosInstance.get( | |
| `/api/v1/cms/sub-tasks/${subtaskId}/preset` | |
| ); |
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!
| await axiosInstance.delete(`/api/v1/cms/sub-tasks/${subtaskId}/preset`, { | ||
| headers: getAuthHeaders(), | ||
| }); |
There was a problem hiding this comment.
headers option from deletePreset.
| await axiosInstance.delete(`/api/v1/cms/sub-tasks/${subtaskId}/preset`, { | |
| headers: getAuthHeaders(), | |
| }); | |
| await axiosInstance.delete(`/api/v1/cms/sub-tasks/${subtaskId}/preset`); |
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!
| {/* Version info box */} | ||
| <div className="ml-4 flex items-center gap-2 px-3 py-2 bg-blue-50 dark:bg-blue-900/20 rounded-md border border-blue-200 dark:border-blue-800"> | ||
| <span className="text-xs font-semibold text-blue-600 dark:text-blue-400 uppercase tracking-wide"> | ||
| {preset.language} | ||
| </span> | ||
| <span className="text-xs text-gray-500 dark:text-gray-400">•</span> | ||
| <span className="text-sm text-gray-700 dark:text-gray-300 truncate max-w-[200px]" title={preset.version_id}> |
There was a problem hiding this comment.
SourceSubtask shows preset.version_id.substring(0, 8) + "..." (a raw UUID prefix) and uses the full UUID as the tooltip. The read-only SourceReferenceWithVersion in TaskView.tsx fetches fetchLanguageVersions and resolves the human-readable version.title. Users editing a subtask see an opaque identifier while the view mode shows the real title — this inconsistency makes the edit card significantly less useful.
| queryClient.invalidateQueries({ queryKey: ["preset", subtaskId] }); | ||
|
|
||
| toast.success("Version preset saved successfully!"); | ||
| onOpenChange(false); | ||
| if (onSuccess) { | ||
| onSuccess(); | ||
| } | ||
| } catch (error: any) { | ||
| toast.error(error?.response?.data?.detail || "Failed to save preset"); | ||
| } finally { | ||
| setIsSaving(false); | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
invalidateQueries on save
handleSave calls queryClient.invalidateQueries({ queryKey: ["preset", subtaskId] }) and then invokes onSuccess, which in TaskView.tsx issues the same invalidation again. The duplicate call is harmless but implies the invalidation inside handleSave is unnecessary — the onSuccess callback is already responsible for cache cleanup and closing the modal, so the inline invalidation here can be removed.
2 participants
No description provided.