-
Notifications
You must be signed in to change notification settings - Fork 7
Add dashboard summary, and improve test coverage and linting #27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Removed DB_FILE environment variable from .env.sample - Deleted legacy investec.db file - Cleaned up commented DB_FILE reference in app.ts
* Add profile and KYC compliant fields to accounts - Added Profile model with profileId and profileName - Updated Account model with kycCompliant, profileId, and profileName fields - Created profile seeding script with default profile (ID: 10001234567890, Name: Joe Soap) - Updated account seeding to include profile references and KYC compliance - Added database journal files to gitignore - Applied Prisma migration to update existing database This ensures production-like account responses for strict validation compatibility. * Add updated database, edit migration
* Add profile and KYC compliant fields to accounts - Added Profile model with profileId and profileName - Updated Account model with kycCompliant, profileId, and profileName fields - Created profile seeding script with default profile (ID: 10001234567890, Name: Joe Soap) - Updated account seeding to include profile references and KYC compliance - Added database journal files to gitignore - Applied Prisma migration to update existing database This ensures production-like account responses for strict validation compatibility. * Add updated database, edit migration * Add real-time database summary dashboard (#3) * Add real-time database summary dashboard with Socket.IO - Added Database Summary section showing live counts for profiles, accounts, cards, and transactions - Implemented Socket.IO real-time updates for all database operations (create/delete accounts, transactions, clear/restore) - Added /database-summary API endpoint with real-time emission via emitDatabaseSummary() - Improved server logging to use req.originalUrl for complete path details - Excluded internal database-summary calls from server logs to reduce noise - Added .idea folder to .gitignore * Update dev.db with test accounts and transactions
* Improve test coverage and robustness - Update account.spec.ts to include new profile fields (kycCompliant, profileId, profileName) - Improve card.spec.ts with better timestamp validation instead of exact matching - Enhance environmentalvariables.spec.ts with comprehensive structure validation - Add new profile.spec.ts with comprehensive profile functionality testing - Remove unnecessary comments from test files for cleaner code - Fix TypeScript warnings with proper parameter types * Remove remaining comments from test files - Clean up card.spec.ts by removing unnecessary comments - Clean up environmentalvariables.spec.ts by removing unnecessary comments - Keep code clean and concise without explanatory comments * Fix ESLint configuration and resolve linting issues - Fix eslint.config.js max-len rule configuration with proper severity and options - Fix Array() constructor usage in cards.ts to use array literal notation - Fix line length issues in card.spec.ts by breaking long strings into concatenated lines - Replace 'any' type with proper typing in profile.spec.ts - All 26 tests still passing - No ESLint errors remaining
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis update introduces a new "Profile" entity to the application, linking it to accounts with profile data and a KYC compliance flag. The database schema, seeding scripts, API endpoints, and tests are updated accordingly. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant Database
User->>Frontend: Request database summary (UI load or after reset)
Frontend->>Backend: GET /database-summary
Backend->>Database: Query counts (profiles, accounts, cards, transactions)
Database-->>Backend: Return counts
Backend-->>Frontend: Respond with summary data
Frontend->>User: Display summary
Note over Backend,Frontend: On database mutation (e.g. account create/delete)
Backend->>Backend: emitDatabaseSummary()
Backend->>Database: Query updated counts
Database-->>Backend: Return new counts
Backend-->>Frontend: WebSocket emit 'database-summary'
Frontend->>User: Update summary in real-time
Possibly related PRs
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -0,0 +1,28 @@ | |||
| -- CreateTable | |||
| CREATE TABLE "Profile" ( | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the migrations could be compressed, given that the DB is shipped with the simulator.
| createdAt: '2024-08-01T13:05:33.685Z', | ||
| updatedAt: '2024-08-01T13:05:33.685Z', | ||
| publishedAt: '2024-08-01T13:05:33.685Z', | ||
| code: '// This function runs before a transaction.\n' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a linting change from the linter, which may be undesirable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/routes/accounts.ts (1)
274-300: Fix duplicate route definition.There are two identical POST routes for
/:accountId/transactionsdefined at lines 274 and 302. This will cause the second route to be unreachable and could lead to unexpected behavior.Remove the duplicate route definition:
-router.post('/:accountId/transactions', async (req: Request, res: Response) => { - try { - let randomTx = Investec.transaction(req.params.accountId) - randomTx.runningBalance = 0 - randomTx = { ...randomTx, ...req.body } - - const accountId = req.params.accountId - // check that the account exists - const account = await prisma.account.findFirst({ - where: { - accountId: accountId, - }, - }) - if (!account) { - console.log('no account found') - return formatErrorResponse(req, res, 404) // no account was found - } - // insert the transaction - const transaction = await prisma.transaction.create({ - data: randomTx, - }) - return formatResponse(transaction, req, res) - } catch (error) { - console.log(error) - return formatErrorResponse(req, res, 500) - } -})Also applies to: 302-328
🧹 Nitpick comments (1)
src/routes/accounts.ts (1)
294-294: Good integration of real-time updates, but consider error handling.The
emitDatabaseSummary()calls are properly positioned after database mutations to ensure clients receive updated summary data. However, consider adding error handling since these calls shouldn't fail the main operation.- await emitDatabaseSummary() + try { + await emitDatabaseSummary() + } catch (error) { + console.log('Failed to emit database summary:', error) + }Also applies to: 352-352, 381-381, 415-415
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
investec.dbis excluded by!**/*.dbprisma/dev.dbis excluded by!**/*.db
📒 Files selected for processing (16)
.env.sample(0 hunks).gitignore(1 hunks)eslint.config.js(1 hunks)prisma/account.ts(1 hunks)prisma/migrations/20250708211014_add_profile_and_kyc/migration.sql(1 hunks)prisma/profile.ts(1 hunks)prisma/schema.prisma(1 hunks)prisma/seed.ts(2 hunks)src/app.ts(5 hunks)src/index.html(5 hunks)src/routes/accounts.ts(5 hunks)src/routes/cards.ts(1 hunks)test/account.spec.ts(1 hunks)test/card.spec.ts(1 hunks)test/environmentalvariables.spec.ts(1 hunks)test/profile.spec.ts(1 hunks)
💤 Files with no reviewable changes (1)
- .env.sample
🧰 Additional context used
🧬 Code Graph Analysis (4)
prisma/seed.ts (1)
prisma/profile.ts (1)
seedProfiles(12-19)
src/routes/accounts.ts (1)
src/app.ts (1)
emitDatabaseSummary(254-274)
test/profile.spec.ts (1)
src/app.ts (1)
app(32-32)
src/app.ts (6)
prisma/profile.ts (1)
seedProfiles(12-19)prisma/account.ts (1)
seedAccounts(58-65)prisma/transaction.ts (1)
seedTransactions(205-212)prisma/beneficiary.ts (1)
seedBeneficiaries(56-63)prisma/card.ts (1)
seedCards(20-27)prisma/card-code.ts (1)
seedCardCodes(22-29)
🔇 Additional comments (30)
eslint.config.js (1)
12-12: LGTM! Improved ESLint rule configuration.The change from a simple numeric value to an explicit error-level configuration with an options object follows ESLint best practices and makes the configuration more maintainable. The 120-character line length limit is appropriate for code readability.
src/routes/cards.ts (1)
13-13: LGTM: Good stylistic improvementReplacing
Array()with the array literal[]is a best practice that improves code readability and follows standard JavaScript conventions..gitignore (1)
5-8: LGTM: Appropriate gitignore additionsThe new entries are well-chosen:
**.db-journalexcludes SQLite journal files (transient database files)CLAUDE.mdexcludes what appears to be AI-generated documentation.ideaexcludes JetBrains IDE configuration filesThese additions follow standard practices for excluding temporary and IDE-specific files.
test/environmentalvariables.spec.ts (1)
11-19: LGTM: Improved test robustnessThe refactored assertions are more maintainable and robust:
- Property existence checks (
assert.exists,assert.property) are more resilient than strict equality- Date parsing validation (
new Date()) is more appropriate than exact timestamp matching- Tests now focus on data structure and type correctness rather than exact values
This approach reduces test brittleness while maintaining coverage of the essential functionality.
test/card.spec.ts (2)
23-50: LGTM: Well-structured test improvementsThe test restructuring provides several benefits:
- Clearer separation between static data validation and dynamic timestamp validation
- Improved readability with explicit newline concatenation in the expected code string
- More robust timestamp validation using date parsing instead of exact string matches
- Better maintainability by focusing on data structure rather than exact values
56-82: LGTM: Consistent test pattern appliedThe same improved testing pattern is consistently applied to the published code test, maintaining code consistency and providing the same benefits as the previous test case.
test/account.spec.ts (1)
15-17: LGTM: Profile and KYC fields additionThe new properties align well with the PR objectives:
kycCompliant: Boolean flag for KYC compliance tracking (essential for banking apps)profileIdandprofileName: Support the new profile functionality mentioned in the PR summaryThe test data appears reasonable and consistent with the broader profile feature integration.
prisma/seed.ts (2)
5-5: LGTM: Proper import integration.The import statement correctly brings in the
seedProfilesfunction from the new profile module.
20-20: LGTM: Correct seeding order.The
seedProfiles()call is properly positioned beforeseedAccounts(), ensuring profile data exists before accounts are created that reference it.prisma/schema.prisma (1)
10-13: LGTM: Clean Profile model definition.The Profile model is well-defined with appropriate primary key and naming.
prisma/profile.ts (1)
1-19: LGTM: Well-structured profile seeding implementation.The implementation follows proper Prisma patterns:
- Correct imports and type usage
- Consistent with other seeding files
- Proper async/await handling
- Clear console logging
The single hardcoded profile is appropriate for testing purposes.
src/routes/accounts.ts (1)
6-6: LGTM: Proper import integration.The import statement correctly adds the
emitDatabaseSummaryfunction for real-time database summary updates.src/index.html (5)
69-85: LGTM! Well-structured database summary UI.The Database Summary section is well-implemented with proper HTML structure and consistent styling. The use of conditional rendering with
|| 0ensures graceful handling of undefined values.
166-171: LGTM! Proper Vue.js data model extension.The summary object is correctly initialized with all required fields (profiles, accounts, cards, transactions) set to zero, following Vue.js best practices.
194-198: LGTM! Proper socket event handling.The socket event listener for 'database-summary' is correctly implemented and follows the existing pattern for handling socket events.
216-222: LGTM! Clean API integration.The
fetchSummary()method follows the same pattern asfetchEnv(), with proper error handling through the fetch API and consistent data assignment.
240-241: LGTM! Proper integration with existing actions.The delayed
fetchSummary()calls after clearing/restoring the database are appropriate, allowing time for the backend operations to complete before refreshing the summary.Also applies to: 248-249
prisma/migrations/20250708211014_add_profile_and_kyc/migration.sql (3)
1-5: LGTM! Proper table creation.The Profile table is correctly defined with appropriate data types and primary key constraint.
10-28: LGTM! Proper SQLite table recreation pattern.The table recreation follows the correct SQLite pattern with proper foreign key handling using PRAGMA commands. The data migration correctly preserves existing data while adding new fields with appropriate defaults.
7-8: Default profile data is consistent.
The migration’s INSERT for profileId10001234567890/ profileNameJoe Soapmatches the seed values inprisma/profile.ts. No changes required.test/profile.spec.ts (4)
1-4: LGTM! Proper test setup.The imports and setup are correct, properly importing the necessary testing libraries and the Express app for testing.
5-22: LGTM! Comprehensive database summary endpoint testing.The test properly validates the
/database-summaryendpoint response structure, ensuring all required fields are present and have correct types. The use ofisAtLeast(0)for profile count is appropriate.
24-44: LGTM! Thorough account profile validation.The test properly validates that accounts contain the new profile fields with correct types. The regex pattern
/^1000\d{10}$/forprofileIdvalidation is appropriate and the conditional testing for array length > 0 prevents false failures.
46-62: LGTM! Good consistency validation.The test ensures profile data consistency across multiple accounts, which is important for data integrity. The type assertion pattern is properly handled for TypeScript compatibility.
src/app.ts (6)
25-25: LGTM! Proper import addition.The import for
seedProfilesis correctly added and consistent with other seeding function imports.
70-71: LGTM! Proper profile integration in database operations.The profile deletion and seeding are correctly integrated into the clear and restore operations, maintaining consistency with other entity operations. The
emitDatabaseSummary()calls ensure real-time updates.Also applies to: 79-80, 86-86
210-231: LGTM! Efficient database summary endpoint.The endpoint uses
Promise.all()for parallel database counting, which is efficient. The response format follows the existing pattern usingformatResponse().
254-274: LGTM! Well-designed WebSocket emission function.The
emitDatabaseSummary()function is properly exported and mirrors the HTTP endpoint logic. Error handling is appropriate with console logging.
278-292: LGTM! Improved logging behavior.The conditional logging exclusion for
/database-summarycalls is a good UX improvement, preventing log clutter from frequent dashboard updates. The switch fromreq.urltoreq.originalUrlprovides more accurate request path logging.
311-311: LGTM! Consistent logging improvement.The change from
req.urltoreq.originalUrlin error response logging maintains consistency with the formatResponse function change.
| kycCompliant: true, | ||
| profileId: '10001234567890', | ||
| profileName: 'Joe Soap', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider normalization and data consistency improvements.
All accounts are using the same profile data, which may be appropriate for testing but raises some concerns:
- Data duplication: Storing both
profileIdandprofileNamein the Account model duplicates data from the Profile table, violating normalization principles. - Data consistency risk: Without foreign key constraints, there's no guarantee the
profileIdexists in the Profile table.
Consider removing profileName from the Account model and using joins to retrieve profile information when needed, or adding proper foreign key relationships.
- kycCompliant: true,
- profileId: '10001234567890',
- profileName: 'Joe Soap',
+ kycCompliant: true,
+ profileId: '10001234567890',Also applies to: 22-24, 32-34, 42-44, 52-54
🤖 Prompt for AI Agents
In prisma/account.ts around lines 12-14, 22-24, 32-34, and 42-44, the Account
model includes both profileId and profileName, causing data duplication and
risking inconsistency. To fix this, remove the profileName field from the
Account model and ensure profileId is a foreign key referencing the Profile
table. Adjust queries to join with the Profile table to retrieve profileName
when needed, maintaining normalization and data integrity.
| kycCompliant Boolean @default(true) | ||
| profileId String | ||
| profileName String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add foreign key relationship and consider normalization.
The Account model extensions introduce profile functionality but lack proper relational constraints:
- Missing foreign key: No relationship defined between
Account.profileIdandProfile.profileId - Data duplication:
profileNameis stored in both Profile and Account tables
Consider this improved schema design:
kycCompliant Boolean @default(true)
profileId String
- profileName String
+ profile Profile @relation(fields: [profileId], references: [profileId])This approach:
- Establishes proper foreign key constraints
- Eliminates data duplication
- Ensures referential integrity
- Requires joining to get profile name when needed
📝 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.
| kycCompliant Boolean @default(true) | |
| profileId String | |
| profileName String | |
| kycCompliant Boolean @default(true) | |
| profileId String | |
| profile Profile @relation(fields: [profileId], references: [profileId]) |
🤖 Prompt for AI Agents
In prisma/schema.prisma around lines 21 to 23, the Account model has profileId
and profileName fields but lacks a foreign key relationship to the Profile
model, causing data duplication and missing referential integrity. To fix this,
remove the profileName field from Account, define a relation field linking
Account.profileId to Profile.id (or the appropriate primary key), and add the
@relation attribute to enforce the foreign key constraint. This will normalize
the schema, eliminate duplicated profileName data, and ensure proper relational
integrity.
|
@stevebissett, is there anything you need from me to get this one merged? |
|
@devinpearson - No - the outstanding but was to update the UI image that shows within the app, that can be done as a follow up. The only other thing is for you to merge it (I don't have permissions to do that). |
- Client Secret and API Key fields now default to hidden state for security - Toggle buttons positioned to the right of input fields with eye icons - Client Secret uses password/text input type switching - API Key uses conditional rendering between textarea and masked dots display - Updated dashboard screenshot to reflect new UI changes
- Implements GET /health endpoint returning HTTP 200 with { status: 'ok' }
- Provides simple service availability check for monitoring tools
- Lightweight response with no database dependencies
- Fixed typo: "room of the domain" to "root of the domain" - Added /health endpoint to Dashboard section for completeness - Preserved original formatting and structure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
55-58: Fix nested-list indentation to satisfy markdownlint (MD007).Sub-bullets are indented 4 spaces; the rule expects 2. Adjust to keep lint clean:
- - Dashboard view of the server + - Dashboard view of the server … - - Health check endpoint returning service status + - Health check endpoint returning service status
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
58-58: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (1)
README.md (1)
48-48: Good wording fix – “root” is clearer than “room”.
No issues found with this change.
- Added fixed height (300px) scrollable container for server logs - Implemented auto-scroll to bottom when new log entries arrive - Added custom scrollbar styling for better visibility - Removed overflow-hidden from parent container to show scrollbar - Enhanced log item padding and styling for better readability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/index.html (6)
7-22: Broaden scrollbar styling for non-WebKit browsers & remove inline duplicationOnly
::-webkit-scrollbar*rules are provided, so Firefox/Edge (non-Chromium) fall back to default scrollbars.
Also note that.logs-containeralready setsscrollbar-width: autoinline, so the CSS here partially duplicates inline declarations.Consider adding
scrollbar-width/scrollbar-colorfor Gecko engines and moving all scrollbar styling into a single CSS block to avoid scattered inline styles..logs-container { + /* Firefox */ + scrollbar-width: thin; + scrollbar-color: #9ca3af #f1f5f9; }
85-101: Make the summary markup semantic & guard against missing keys
div/spanpairs work, but a definition list or small table would be more accessible (screen-readers announce term/value pairs clearly).
Additionally, if the backend ever omits a field, Vue will renderundefined; the|| 0guard covers numbers but still flashes falsy strings.-<div class="text-sm text-gray-600"> - <span class="font-medium">Profiles:</span> - <span class="font-semibold text-gray-900">{{ summary.profiles ?? 0 }}</span> +<dl class="grid grid-cols-4 gap-x-8 text-sm text-gray-600"> + <dt class="font-medium">Profiles</dt><dd class="font-semibold text-gray-900">{{ summary.profiles ?? 0 }}</dd>(Repeat for other metrics.)
108-115: Conflicting height rules cause double scroll-bars
logs-containerhasheight: 300pxwhile its childuldeclaresmin-height: 350px, guaranteeing overflow even when only a few messages exist.
Drop themin-heightor match the container height to avoid a redundant scrollbar and wasted space.-<ul ... style="min-height: 350px;"> +<ul ...>
240-245:database-summarysocket event already provides fresh data – redundant fetchYou call
fetchSummary()immediately after establishing the socket and again after DB ops, but the server also emits adatabase-summaryevent which you handle. Duplicating viafetch:
- Sends an unnecessary HTTP request.
- Risks race conditions where the GET response arrives before the mutation completes, while the socket event is already the authoritative push channel.
Consider removing the manual fetches and relying solely on the socket, or vice-versa for simpler HTTP-only flow.
286-296: Arbitrary 500 ms delay after clear/restoreWaiting 0.5 s before refreshing assumes the server finishes mutations within that window. Use either:
- A socket ack / event from the server confirming completion, or
- The existing
database-summarypush just discussed,and drop the timeout to guarantee consistency regardless of DB size or load.
304-309: Nit: helpers can be collapsed into one linerBoth visibility toggles are identical; unify into a single method taking the key to flip to keep the methods section concise.
-toggleClientSecretVisibility() { - this.showClientSecret = !this.showClientSecret; -}, -toggleApiKeyVisibility() { - this.showApiKey = !this.showApiKey; -}, +toggleVisibility(key) { + this[key] = !this[key]; +},Called as
toggleVisibility('showClientSecret'), etc.
| showClientSecret: false, | ||
| showApiKey: false, | ||
| summary: { | ||
| profiles: 0, | ||
| accounts: 0, | ||
| cards: 0, | ||
| transactions: 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Secrets live forever in a reactive front-end state
client_secret and api_key are stored in Vue’s reactive data. Anyone with DevTools access (or a JS snippet in console) can read them even when the inputs are masked. If these credentials are meant only for local simulation, note the risk; otherwise move them to an HTTP-only cookie or keep them solely server-side.
🤖 Prompt for AI Agents
In src/index.html around lines 205 to 213, client_secret and api_key are stored
in Vue's reactive data, exposing them to anyone with DevTools access even if
masked. To fix this, remove these secrets from the reactive front-end state and
instead store them securely on the server or in HTTP-only cookies to prevent
client-side access. If they are only for local simulation, clearly document the
risk and avoid using them in production code.
| fetchSummary() { | ||
| fetch('/database-summary') | ||
| .then(response => response.json()) | ||
| .then(data => { | ||
| this.summary = data.data; | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add basic error handling to fetchSummary
A failing /database-summary GET (network error or 5xx) currently throws an unhandled promise rejection. Wrap in try/catch or append .catch() to surface an error toast.
fetch('/database-summary')
.then(r => r.json())
.then(d => { this.summary = d.data })
+ .catch(() => this.toast('Unable to load DB summary'));Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/index.html around lines 262 to 268, the fetchSummary method lacks error
handling for the fetch call to '/database-summary', which can cause unhandled
promise rejections on network or server errors. Add a .catch() block after the
existing .then() chain to handle errors by showing an error toast or logging the
error, ensuring any fetch failures are gracefully handled and surfaced to the
user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (3)
48-48: Nit: be explicit that “root” is/“Root of the domain” can be ambiguous for newcomers. Consider saying “Visiting
/shows the dashboard …” to avoid any doubt.
55-58: Fix markdown-lint indent (MD007)Nested bullets should be indented by 2 spaces, not 4.
- - Dashboard view of the server - - Health check endpoint returning service status + - Dashboard view of the server + - Health check endpoint returning service status
110-113: Same indent issue for the new beneficiary endpoints- - Create a new beneficiary - - Delete a beneficiary + - Create a new beneficiary + - Delete a beneficiary
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(3 hunks)src/routes/accounts.ts(7 hunks)src/routes/cards.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/routes/cards.ts
- src/routes/accounts.ts
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
58-58: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
111-111: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
113-113: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
|
I've added quite a bit more since this and opened #30 with the additional changes from my fork. |
Adds in these from fork:
Add profiles & dashboard summary:
stevebissett#4
Improve test coverage and robustness
stevebissett#5
Outstanding before merge:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Documentation