-
Notifications
You must be signed in to change notification settings - Fork 0
Implement persistent storage for todos and enhance logging: Added fil… #13
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
…e-based data persistence in `todoStore`, updated server to initialize and save todos, and improved logging for request handling. Updated README to reflect new configuration options and functionality.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody Configuration@reviewOptions |
| bun run src/server.ts | ||
| ``` | ||
|
|
||
| > Use `bun run --watch src/server.ts` para modo de desenvolvimento com recarga automática. |
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.
> For development, use `npm start`. For a containerized environment with auto-reloading, use `docker compose up --build`.The project's contribution guidelines require using npm start or docker compose for running the application. The command bun run --watch src/server.ts does not follow these standards. Please update the documentation to reflect the standard commands for development to ensure a consistent workflow.
Kody Rule violation: Use npm start ou docker-start, nunca npm run build
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const shutdown = (signal: string) => { | ||
| console.log(`Recebido ${signal}, finalizando...`); | ||
| try { | ||
| server.stop(); | ||
| } catch {} | ||
| process.exit(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.
const shutdown = (signal: string) => {
console.log(`Received ${signal}, shutting down...`);
server.stop(true);
};The current shutdown implementation uses process.exit(0), which forcefully terminates the application. This is risky because it can interrupt ongoing requests, such as writing a new 'todo' to the database file, leading to data corruption or loss.
To ensure data integrity, the server should shut down gracefully. By using server.stop(true), you allow all in-flight requests to complete before the server stops. The process will then exit cleanly on its own once all operations are finished, preventing any data from being left in an inconsistent state.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| async #save() { | ||
| if (!this.#dataFile) return; | ||
| const data = Array.from(this.#todos.values()).map((t) => ({ | ||
| id: t.id, | ||
| title: t.title, | ||
| completed: t.completed, | ||
| createdAt: t.createdAt.toISOString(), | ||
| updatedAt: t.updatedAt.toISOString(), | ||
| })); | ||
| await this.#ensureDir(); | ||
| await Bun.write(this.#dataFile, JSON.stringify(data, null, 2) + "\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.
#savePromise: Promise<void> = Promise.resolve();
#save() {
this.#savePromise = this.#savePromise
.then(() => this.#doSave())
.catch((err) => {
console.error("Error saving data:", err);
});
}
async #doSave() {
if (!this.#dataFile) return;
const data = Array.from(this.#todos.values()).map((t) => ({
id: t.id,
title: t.title,
completed: t.completed,
createdAt: t.createdAt.toISOString(),
updatedAt: t.updatedAt.toISOString(),
}));
await this.#ensureDir();
await Bun.write(this.#dataFile, JSON.stringify(data, null, 2) + "
");
}The current implementation of the #save() method is invoked without being awaited in create, update, and delete. This fire-and-forget approach creates a race condition when multiple modifications happen in quick succession. To fix this, all save operations must be serialized to ensure they execute one after another. This can be achieved by chaining promises. A private property #savePromise can hold the promise for the currently executing or last-scheduled save operation. Each new call to #save() chains its work onto this promise, guaranteeing sequential execution and preventing data corruption.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
sartorijr92
left a comment
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.
Found critical issues please review the requested changes
…o support pagination, sorting, and bulk deletion by completion status. Improved JSON body handling with payload size limits and added request ID logging. Updated README to reflect new features and response headers.
This pull request introduces significant enhancements to the Todo API, primarily focusing on data persistence, operational logging, and server robustness.
Key Changes:
File-Based Data Persistence:
todoStorehas been refactored to implement file-based data persistence. Instead of storing todos only in memory, they are now loaded from and saved to a JSON file on disk.initTodoStorefunction is introduced, which is called at server startup to load existing todos fromdata/todos.jsonby default, or from a path specified by theDATA_FILEenvironment variable. If the file or directory doesn't exist, it's created.create,update, anddeleteoperations on todos now trigger an asynchronous save to the data file, ensuring that changes are persisted across server restarts.createdAt,updatedAt) are serialized to and deserialized from ISO 8601 strings in the JSON file..gitignorefile was updated to includedata/, preventing the persistent data files from being committed to the repository.Enhanced Request Logging:
stdoutwith details including the HTTP method, request path, response status code, and the duration of the request handling in milliseconds.try...catchblock was added around the request handling logic to catch unexpected errors, log them tostderr, and return a500 Internal Server Errorresponse with the request duration.Graceful Server Shutdown:
SIGINT(e.g., Ctrl+C) andSIGTERMsignals.New Root Endpoint:
GET /endpoint has been added, providing a simple welcome message and guiding users to other available routes like/healthand/todos.Updated Documentation:
README.mdhas been comprehensively updated to reflect these changes, including:PORT,DATA_FILE).GET /route.Technical Context:
The persistence mechanism leverages Bun's native file system APIs (
Bun.file,Bun.write) and Node.js'spathandfs/promisesmodules for directory management. The server'sfetchhandler was refactored to encapsulate request processing within ahandlefunction, allowing for consistent logging and error handling usingtry...catchblocks. Environment variables are used for flexible configuration of the server port and data file path.