-
Notifications
You must be signed in to change notification settings - Fork 112
chore: fix resize #190
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
chore: fix resize #190
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnhances logging around HTTP API requests and new page creation, stabilizes a publish-page Cypress test assertion, and slightly adjusts the slash panel database tree item icon layout for better rendering. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- The new
Log.debug('[executeAPIRequest]', { ..., response_data: response.data?.data, ... })call may end up logging large payloads or sensitive data; consider logging only metadata (e.g., sizes, IDs, codes) or guarding it behind a more verbose log level flag. - In
addAppPage, you now log the full request payload includingname; if this can contain user-entered or sensitive content, consider redacting or truncating it before logging.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `Log.debug('[executeAPIRequest]', { ..., response_data: response.data?.data, ... })` call may end up logging large payloads or sensitive data; consider logging only metadata (e.g., sizes, IDs, codes) or guarding it behind a more verbose log level flag.
- In `addAppPage`, you now log the full request payload including `name`; if this can contain user-entered or sensitive content, consider redacting or truncating it before logging.
## Individual Comments
### Comment 1
<location> `src/application/services/js-services/http/http_api.ts:159` </location>
<code_context>
const method = response.config?.method?.toUpperCase() || 'UNKNOWN';
- Log.debug('[executeAPIRequest]', { method, url: requestUrl });
+ Log.debug('[executeAPIRequest]', { method, url: requestUrl, response_data: response.data?.data, response_code: response.data?.code, response_message: response.data?.message });
if (!response.data) {
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider narrowing or guarding the logged response payload to avoid excessive or sensitive data in logs.
Logging `response.data?.data` and `message` risks putting large or sensitive user content into logs. If you only need this for debugging, consider limiting logs to metadata (e.g., status code, flags, payload size), or conditionally logging detailed data behind a debug flag/env check, or truncating/sanitizing the payload first.
```suggestion
Log.debug('[executeAPIRequest]', {
method,
url: requestUrl,
response_code: response.data?.code,
has_data: Boolean(response.data?.data),
message_length: response.data?.message ? String(response.data.message).length : 0,
});
```
</issue_to_address>
### Comment 2
<location> `src/application/services/js-services/http/http_api.ts:1443-1447` </location>
<code_context>
const url = `/api/workspace/${workspaceId}/page-view`;
- return executeAPIRequest<CreatePageResponse>(() =>
+ Log.debug('[addAppPage] request', { url, workspaceId, parentViewId, layout, name });
+
+ const response = await executeAPIRequest<CreatePageResponse>(() =>
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Review whether `name` (and possibly other identifiers) should be logged for privacy and log-noise reasons.
Since `name` may contain user-entered content, logging it alongside `workspaceId` and `parentViewId` can expose user data and add log noise. If these logs are only for operational debugging, consider logging just IDs/layout, or masking/truncating `name` so full user strings aren’t persisted in logs.
```suggestion
const url = `/api/workspace/${workspaceId}/page-view`;
// Avoid logging full user-entered `name` to reduce PII exposure and log noise.
Log.debug('[addAppPage] request', {
url,
workspaceId,
parentViewId,
layout,
hasName: Boolean(name),
nameLength: typeof name === 'string' ? name.length : undefined,
});
const response = await executeAPIRequest<CreatePageResponse>(() =>
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const method = response.config?.method?.toUpperCase() || 'UNKNOWN'; | ||
|
|
||
| Log.debug('[executeAPIRequest]', { method, url: requestUrl }); | ||
| Log.debug('[executeAPIRequest]', { method, url: requestUrl, response_data: response.data?.data, response_code: response.data?.code, response_message: response.data?.message }); |
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.
🚨 suggestion (security): Consider narrowing or guarding the logged response payload to avoid excessive or sensitive data in logs.
Logging response.data?.data and message risks putting large or sensitive user content into logs. If you only need this for debugging, consider limiting logs to metadata (e.g., status code, flags, payload size), or conditionally logging detailed data behind a debug flag/env check, or truncating/sanitizing the payload first.
| Log.debug('[executeAPIRequest]', { method, url: requestUrl, response_data: response.data?.data, response_code: response.data?.code, response_message: response.data?.message }); | |
| Log.debug('[executeAPIRequest]', { | |
| method, | |
| url: requestUrl, | |
| response_code: response.data?.code, | |
| has_data: Boolean(response.data?.data), | |
| message_length: response.data?.message ? String(response.data.message).length : 0, | |
| }); |
| const url = `/api/workspace/${workspaceId}/page-view`; | ||
|
|
||
| return executeAPIRequest<CreatePageResponse>(() => | ||
| Log.debug('[addAppPage] request', { url, workspaceId, parentViewId, layout, name }); | ||
|
|
||
| const response = await executeAPIRequest<CreatePageResponse>(() => |
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.
🚨 suggestion (security): Review whether name (and possibly other identifiers) should be logged for privacy and log-noise reasons.
Since name may contain user-entered content, logging it alongside workspaceId and parentViewId can expose user data and add log noise. If these logs are only for operational debugging, consider logging just IDs/layout, or masking/truncating name so full user strings aren’t persisted in logs.
| const url = `/api/workspace/${workspaceId}/page-view`; | |
| return executeAPIRequest<CreatePageResponse>(() => | |
| Log.debug('[addAppPage] request', { url, workspaceId, parentViewId, layout, name }); | |
| const response = await executeAPIRequest<CreatePageResponse>(() => | |
| const url = `/api/workspace/${workspaceId}/page-view`; | |
| // Avoid logging full user-entered `name` to reduce PII exposure and log noise. | |
| Log.debug('[addAppPage] request', { | |
| url, | |
| workspaceId, | |
| parentViewId, | |
| layout, | |
| hasName: Boolean(name), | |
| nameLength: typeof name === 'string' ? name.length : undefined, | |
| }); | |
| const response = await executeAPIRequest<CreatePageResponse>(() => |
Description
Checklist
General
Testing
Feature-Specific
Summary by Sourcery
Improve logging for page creation and API responses, adjust UI layout for database items in the slash panel, and stabilize a publish page E2E test.
Bug Fixes:
Enhancements:
Tests: