Feat/toggl row collapsing#1118
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds a new CSV row collapsing setting with persisted user profile storage, validation support, and a new error constant. It also wires the setting into account UI, API updates, and CSV download behavior. ChangesCSV row collapsing setting
Sequence DiagramsequenceDiagram
actor User
participant AccountPage as Account page
participant ApiUser as PUT /api/user
participant UserService as updateCsvRowCollapsing
participant Prisma as UserProfile table
User->>AccountPage: Toggle CSV row collapsing
AccountPage->>ApiUser: PUT csvRowCollapsing
ApiUser->>UserService: updateCsvRowCollapsing(id, value)
UserService->>Prisma: update csvRowCollapsing
Prisma-->>UserService: updated row
UserService-->>ApiUser: success
ApiUser-->>AccountPage: { success: true }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/Account/CsvRowCollapsing.tsx`:
- Around line 24-31: The current handler only reverts the optimistic toggle in
the catch block and assumes axios-style errors; update the logic around the
fetch response (variable res) so non-2xx responses are handled: after awaiting
the fetch response check res.ok, and if false parse the response body (await
res.json() or text) to extract an error message, call setError(...) with that
message, clear setSuccess(''), and revert the optimistic change via
setCsvRowCollapsing(!newValue); otherwise on success call setError('') and
setSuccess('Updated successfully.'); also adjust the catch block to use a
generic fallback message (err.message) for network failures.
In `@pages/api/user/index.ts`:
- Around line 17-22: Wrap the call to parseUpdateCsvRowCollapsingPUTRequest(...)
in a try/catch similar to the existing parseUpdatePUTRequest handling: call
parseUpdateCsvRowCollapsingPUTRequest(req.body) inside try, and on validation
error catch it and respond with res.status(400).json({ success: false, error:
"INVALID_CSV_ROW_COLLAPSING_400" }); otherwise proceed to await
userService.updateCsvRowCollapsing(session.userId, csvRowCollapsing) and return
200. Ensure you only catch validation errors thrown by
parseUpdateCsvRowCollapsingPUTRequest and keep other errors unhandled or
rethrown as appropriate.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
components/Account/CsvRowCollapsing.tsxcomponents/Account/account.module.cssconstants/index.tspages/account/account.module.csspages/account/index.tsxpages/api/paybutton/download/transactions/[paybuttonId].tspages/api/payments/download/index.tspages/api/user/index.tsprisma-local/migrations/20260228024317_csv_row_collapsing/migration.sqlprisma-local/schema.prismaservices/userService.tstests/mockedObjects.tsutils/validators.ts
Comment on lines
+24
to
+31
| if (res.status === 200) { | ||
| setError('') | ||
| setSuccess('Updated successfully.') | ||
| } | ||
| } catch (err: any) { | ||
| setSuccess('') | ||
| setError(err.response?.data?.message ?? 'Failed to update setting.') | ||
| setCsvRowCollapsing(!newValue) |
Contributor
There was a problem hiding this comment.
Non-2xx HTTP responses won't revert the optimistic update.
The fetch API doesn't throw on HTTP error statuses (4xx, 5xx), so the catch block only handles network failures. When the API returns an error status, the UI will remain in the toggled state without showing an error message.
Additionally, err.response?.data?.message is an axios-style pattern; native fetch errors don't have this structure.
🐛 Proposed fix to handle non-2xx responses
try {
const res = await fetch('/api/user', {
method: 'PUT',
headers: {
'Content-Type': 'application/json'
},
body: JSON.stringify({ csvRowCollapsing: newValue })
})
if (res.status === 200) {
setError('')
setSuccess('Updated successfully.')
+ } else {
+ const data = await res.json().catch(() => ({}))
+ setSuccess('')
+ setError(data.message ?? 'Failed to update setting.')
+ setCsvRowCollapsing(!newValue)
}
} catch (err: any) {
setSuccess('')
- setError(err.response?.data?.message ?? 'Failed to update setting.')
+ setError('Failed to update setting.')
setCsvRowCollapsing(!newValue)
} finally {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (res.status === 200) { | |
| setError('') | |
| setSuccess('Updated successfully.') | |
| } | |
| } catch (err: any) { | |
| setSuccess('') | |
| setError(err.response?.data?.message ?? 'Failed to update setting.') | |
| setCsvRowCollapsing(!newValue) | |
| if (res.status === 200) { | |
| setError('') | |
| setSuccess('Updated successfully.') | |
| } else { | |
| const data = await res.json().catch(() => ({})) | |
| setSuccess('') | |
| setError(data.message ?? 'Failed to update setting.') | |
| setCsvRowCollapsing(!newValue) | |
| } | |
| } catch (err: any) { | |
| setSuccess('') | |
| setError('Failed to update setting.') | |
| setCsvRowCollapsing(!newValue) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/Account/CsvRowCollapsing.tsx` around lines 24 - 31, The current
handler only reverts the optimistic toggle in the catch block and assumes
axios-style errors; update the logic around the fetch response (variable res) so
non-2xx responses are handled: after awaiting the fetch response check res.ok,
and if false parse the response body (await res.json() or text) to extract an
error message, call setError(...) with that message, clear setSuccess(''), and
revert the optimistic change via setCsvRowCollapsing(!newValue); otherwise on
success call setError('') and setSuccess('Updated successfully.'); also adjust
the catch block to use a generic fallback message (err.message) for network
failures.
Comment on lines
+17
to
+22
| if (req.body.csvRowCollapsing !== undefined) { | ||
| const { csvRowCollapsing } = parseUpdateCsvRowCollapsingPUTRequest(req.body) | ||
| await userService.updateCsvRowCollapsing(session.userId, csvRowCollapsing) | ||
| res.status(200).json({ success: true }) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
Missing error handling for validation failures.
If parseUpdateCsvRowCollapsingPUTRequest throws (when csvRowCollapsing is not a boolean), the error will propagate as an unhandled exception, resulting in a 500 response instead of the intended 400 status with INVALID_CSV_ROW_COLLAPSING_400.
Note: This mirrors the existing pattern for parseUpdatePUTRequest (line 23), so it's consistent but worth addressing for proper error responses.
🛡️ Proposed fix to add error handling
if (req.method === 'PUT') {
if (req.body.csvRowCollapsing !== undefined) {
+ try {
const { csvRowCollapsing } = parseUpdateCsvRowCollapsingPUTRequest(req.body)
await userService.updateCsvRowCollapsing(session.userId, csvRowCollapsing)
res.status(200).json({ success: true })
return
+ } catch (error: any) {
+ res.status(400).json({ message: error.message })
+ return
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (req.body.csvRowCollapsing !== undefined) { | |
| const { csvRowCollapsing } = parseUpdateCsvRowCollapsingPUTRequest(req.body) | |
| await userService.updateCsvRowCollapsing(session.userId, csvRowCollapsing) | |
| res.status(200).json({ success: true }) | |
| return | |
| } | |
| if (req.body.csvRowCollapsing !== undefined) { | |
| try { | |
| const { csvRowCollapsing } = parseUpdateCsvRowCollapsingPUTRequest(req.body) | |
| await userService.updateCsvRowCollapsing(session.userId, csvRowCollapsing) | |
| res.status(200).json({ success: true }) | |
| return | |
| } catch (error: any) { | |
| res.status(400).json({ message: error.message }) | |
| return | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pages/api/user/index.ts` around lines 17 - 22, Wrap the call to
parseUpdateCsvRowCollapsingPUTRequest(...) in a try/catch similar to the
existing parseUpdatePUTRequest handling: call
parseUpdateCsvRowCollapsingPUTRequest(req.body) inside try, and on validation
error catch it and respond with res.status(400).json({ success: false, error:
"INVALID_CSV_ROW_COLLAPSING_400" }); otherwise proceed to await
userService.updateCsvRowCollapsing(session.userId, csvRowCollapsing) and return
200. Ensure you only catch validation errors thrown by
parseUpdateCsvRowCollapsingPUTRequest and keep other errors unhandled or
rethrown as appropriate.
Klakurka
approved these changes
Jun 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to #953
Description
This PR introduces configurability for CSV row collapsing behavior.
Changes
Added a new prop to UserProfile to control CSV row collapsing behavior.
Added a toggle setting in Account Settings to allow users to define their preferred row collapsing behavior.
Refactored row collapsing logic to be prop driven instead of predefined (hardcoded).
Row collapsing behavior is now fully configurable and controlled via user settings, improving flexibility and user customization.
Test plan
Test CSV row collapsing by enabling and disabling the toggle in Account Settings, and verify that the exported CSV reflects the selected configuration correctly.
Summary by CodeRabbit
New Features
UI Improvements
Backend Support