Add task preset feature with security review docs and UX improvements#2
Draft
manuelgruber wants to merge 6 commits intomainfrom
Draft
Add task preset feature with security review docs and UX improvements#2manuelgruber wants to merge 6 commits intomainfrom
manuelgruber wants to merge 6 commits intomainfrom
Conversation
Bumps React to 19, react-router-dom to v7, date-fns to v4, and all associated type definitions and dev tooling to current releases.
Introduces the TaskPreset table for storing reusable task templates. Adds TaskPresetService with CRUD operations and a preset_exists_by_name guard used during seeding. run_migration now seeds seven built-in presets (reserve levels, grid charging, operating modes, grid export) on startup if they are not already present.
Adds GET /tasks/presets, POST /tasks/presets, and DELETE /tasks/presets/<id> under the tasks blueprint. Built-in presets are protected from deletion (403). Input is validated against CommandType and CronCommand before persisting.
Planner page gains a preset selector (grouped by built-in vs user), "Save as Preset" modal, and delete button for user-defined presets. TaskPreset type and three api client methods (getTaskPresets, createTaskPreset, deleteTaskPreset) are added. Responsive fixes across the app: mobile hamburger nav in Header, horizontal scroll on LogsTable, touch-dismiss support in Tooltip, flex-wrap on Dashboard status bar and Settings form rows, and stacked action buttons in the Planner task table on small screens.
WalkthroughThis pull request introduces task preset management to PowerNight: a new database model, service layer, and API endpoints enable users to create, list, and delete reusable task templates. Frontend updates add responsive design improvements to multiple components and pages, along with dependency upgrades. Documentation plans for security hardening and UX improvements are also included. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Planner as Planner Page
participant API as API Routes
participant Service as TaskPresetService
participant DB as Database
User->>Planner: Load page / Select preset / Save as preset / Delete preset
Planner->>API: GET /api/v1/tasks/presets
API->>Service: list_presets()
Service->>DB: Query TaskPreset records
DB-->>Service: Return presets (ordered by sort_order, name)
Service-->>API: Return presets array + total
API-->>Planner: Return { success, presets, total }
User->>Planner: Fill form & click "Save as Preset"
Planner->>API: POST /api/v1/tasks/presets (name, command, command_params, default_time)
API->>API: Validate command, command_params, default_time
API->>Service: create_preset(...)
Service->>DB: Insert TaskPreset with is_builtin=false
DB-->>Service: Return created preset
Service-->>API: Return created preset dict
API-->>Planner: Return { success, preset }
Planner->>Planner: Refresh preset list, clear form
User->>Planner: Select preset to delete
Planner->>API: DELETE /api/v1/tasks/presets/<preset_id>
API->>Service: delete_preset(preset_id)
Service->>DB: Check is_builtin flag
alt is_builtin == true
Service-->>API: Raise DatabaseError (blocked)
API-->>Planner: Return { success: false, error: "...", status: 403 }
else is_builtin == false
DB->>DB: Delete preset record
Service-->>API: Return true
API-->>Planner: Return { success: true, status: 200 }
end
Planner->>Planner: Refresh presets, clear selection if needed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/powernight/core/database/services.py (1)
779-780: Consider preserving exception chain for debugging.The exception re-raising pattern loses the original traceback. Using
raise ... from ewould help with debugging.♻️ Optional improvement
except Exception as e: - raise DatabaseError(f"Failed to create preset: {e}") + raise DatabaseError(f"Failed to create preset: {e}") from eApply the same pattern to the other
except Exception as eblocks in this service.Also applies to: 790-791, 805-806, 824-825
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/powernight/core/database/services.py` around lines 779 - 780, The DatabaseError raised in the except blocks currently discards the original traceback; update the re-raises to preserve the exception chain by using "raise DatabaseError(f'Failed to create preset: {e}') from e" (replace the message as appropriate) in the except block shown and apply the same pattern to the other handlers in this service (the other except Exception as e blocks that raise DatabaseError around the create/preset logic) so the original exception is chained to DatabaseError for proper debugging.src/powernight/web/api/tasks_api.py (1)
802-809: Fragile string matching to detect builtin preset deletion.The check
if "Built-in" in str(e)couples the API layer to the exact error message text fromTaskPresetService.delete_preset(). If the message changes, this will return 500 instead of 403.Consider introducing a specific exception type (e.g.,
BuiltinPresetError) for this case.♻️ Suggested approach
In
exceptions.py:class BuiltinPresetError(DatabaseError): """Raised when attempting to modify a built-in preset.""" passIn
services.py:if preset.is_builtin: - raise DatabaseError("Built-in presets cannot be deleted") + raise BuiltinPresetError("Built-in presets cannot be deleted")In
tasks_api.py:+ except BuiltinPresetError as e: + return jsonify({ + 'success': False, + 'error': 'Forbidden', + 'message': str(e), + 'timestamp': datetime.now(timezone.utc).isoformat() + }), 403 + except DatabaseError as e: - if "Built-in" in str(e): - return jsonify({ - 'success': False, - 'error': 'Forbidden', - 'message': str(e), - 'timestamp': datetime.now(timezone.utc).isoformat() - }), 403 return jsonify({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/powernight/web/api/tasks_api.py` around lines 802 - 809, Replace the fragile string-match by introducing a dedicated exception and using it end-to-end: add BuiltinPresetError(DatabaseError) in exceptions.py, update TaskPresetService.delete_preset (or the method that raises DatabaseError on built-in deletion) to raise BuiltinPresetError instead of raising DatabaseError with the "Built-in" text, then in tasks_api.py import BuiltinPresetError and change the handler to except BuiltinPresetError as e: return the 403 JSON response (and keep a general except DatabaseError fallback for other DB errors); ensure imports are updated accordingly.src/powernight/web/src/pages/Planner.tsx (1)
69-76: Consider surfacing preset loading errors to users.Unlike
handleSavePresetandhandleDeletePresetwhich set the error state,loadPresetssilently logs errors to the console. If presets fail to load, users won't see any indication.♻️ Optional: Add error feedback
const loadPresets = async () => { try { const data = await api.getTaskPresets(); setPresets(data.presets || []); } catch (err) { console.error('Failed to load presets:', err); + // Optionally notify user, though this is non-critical + // setError('Failed to load presets'); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/powernight/web/src/pages/Planner.tsx` around lines 69 - 76, loadPresets currently swallows errors by only logging to console; update its error handling to mirror handleSavePreset/handleDeletePreset by calling the component error setter (e.g., setError) inside the catch with a user-facing message (and optionally the err.message), and clear that error on successful load (e.g., call setError(null/undefined) before or after setPresets). Specifically modify the loadPresets function that calls api.getTaskPresets to setPresets(data.presets || []) on success and setError(...) on failure so users receive visible feedback when presets fail to load.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/powernight/web/src/components/Header.tsx`:
- Around line 55-60: The mobile nav toggle removed the browser focus ring making
keyboard use impossible; in the Header component update the toggle button
(onClick={() => setMenuOpen(!menuOpen)}, aria-expanded={menuOpen}) and the
mobile menu items to restore a visible keyboard focus state instead of using
focus:outline-none — either remove that utility or replace it with an accessible
focus style (e.g. focus-visible:ring / focus-visible:outline or a clear
background/border change) and apply the same visible focus treatment to the
corresponding menu item elements rendered in the collapse (also present around
the elements referenced near the other block at lines showing the same pattern).
Ensure the aria-expanded logic and setMenuOpen remain unchanged while only
altering classes/styles for keyboard focus visibility.
In `@src/powernight/web/src/components/Tooltip.tsx`:
- Around line 112-124: The touch handler handleTouchStart currently calls
e.preventDefault(), which blocks follow-up clicks on touch devices and breaks
interactive children; remove the e.preventDefault() call (or conditionally skip
it for interactive elements) and simply toggle setIsVisible((prev) => !prev) so
the tooltip still opens on touch without cancelling the native click for
links/buttons wrapped by triggerRef.
In `@src/powernight/web/src/utils/api.ts`:
- Around line 345-356: The createTaskPreset method references the CommandType
type but it isn't imported, causing a TypeScript error; add an import for
CommandType from the module that exports your types (e.g., import { CommandType
} from '../types') at the top of the file so the type annotation on
createTaskPreset is resolved; ensure the import is included alongside other
imports in src/powernight/web/src/utils/api.ts and that the symbol name exactly
matches CommandType used in the function signature.
---
Nitpick comments:
In `@src/powernight/core/database/services.py`:
- Around line 779-780: The DatabaseError raised in the except blocks currently
discards the original traceback; update the re-raises to preserve the exception
chain by using "raise DatabaseError(f'Failed to create preset: {e}') from e"
(replace the message as appropriate) in the except block shown and apply the
same pattern to the other handlers in this service (the other except Exception
as e blocks that raise DatabaseError around the create/preset logic) so the
original exception is chained to DatabaseError for proper debugging.
In `@src/powernight/web/api/tasks_api.py`:
- Around line 802-809: Replace the fragile string-match by introducing a
dedicated exception and using it end-to-end: add
BuiltinPresetError(DatabaseError) in exceptions.py, update
TaskPresetService.delete_preset (or the method that raises DatabaseError on
built-in deletion) to raise BuiltinPresetError instead of raising DatabaseError
with the "Built-in" text, then in tasks_api.py import BuiltinPresetError and
change the handler to except BuiltinPresetError as e: return the 403 JSON
response (and keep a general except DatabaseError fallback for other DB errors);
ensure imports are updated accordingly.
In `@src/powernight/web/src/pages/Planner.tsx`:
- Around line 69-76: loadPresets currently swallows errors by only logging to
console; update its error handling to mirror handleSavePreset/handleDeletePreset
by calling the component error setter (e.g., setError) inside the catch with a
user-facing message (and optionally the err.message), and clear that error on
successful load (e.g., call setError(null/undefined) before or after
setPresets). Specifically modify the loadPresets function that calls
api.getTaskPresets to setPresets(data.presets || []) on success and
setError(...) on failure so users receive visible feedback when presets fail to
load.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9dc854b8-866c-4b0e-a5cc-09913467a6e8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
docs/plans/2026-04-25-security-remediation.mddocs/plans/ux-improvement-roadmap.mdpackage.jsonsrc/powernight/core/database/__init__.pysrc/powernight/core/database/exceptions.pysrc/powernight/core/database/migration.pysrc/powernight/core/database/models.pysrc/powernight/core/database/services.pysrc/powernight/web/api/tasks_api.pysrc/powernight/web/src/components/Header.tsxsrc/powernight/web/src/components/LogsTable.tsxsrc/powernight/web/src/components/Tooltip.tsxsrc/powernight/web/src/pages/Dashboard.tsxsrc/powernight/web/src/pages/Planner.tsxsrc/powernight/web/src/pages/Settings.tsxsrc/powernight/web/src/types/index.tssrc/powernight/web/src/utils/api.ts
Comment on lines
+55
to
+60
| <button | ||
| onClick={() => setMenuOpen(!menuOpen)} | ||
| className="text-white p-2 rounded-md hover:bg-white/10 focus:outline-none" | ||
| aria-label="Toggle navigation menu" | ||
| aria-expanded={menuOpen} | ||
| > |
There was a problem hiding this comment.
Restore visible keyboard focus for the mobile navigation.
The new small-screen nav removes the toggle’s default focus outline and doesn’t add a replacement focus state for the menu items. That makes the collapsed navigation hard to use with a keyboard.
Suggested fix
- className="text-white p-2 rounded-md hover:bg-white/10 focus:outline-none"
+ className="text-white p-2 rounded-md hover:bg-white/10 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-white focus-visible:ring-offset-2 focus-visible:ring-offset-blue-600"
...
- className={`block px-4 py-2 text-sm font-bold transition-colors ${
+ className={`block px-4 py-2 text-sm font-bold transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-inset focus-visible:ring-white/80 ${Also applies to: 82-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/powernight/web/src/components/Header.tsx` around lines 55 - 60, The
mobile nav toggle removed the browser focus ring making keyboard use impossible;
in the Header component update the toggle button (onClick={() =>
setMenuOpen(!menuOpen)}, aria-expanded={menuOpen}) and the mobile menu items to
restore a visible keyboard focus state instead of using focus:outline-none —
either remove that utility or replace it with an accessible focus style (e.g.
focus-visible:ring / focus-visible:outline or a clear background/border change)
and apply the same visible focus treatment to the corresponding menu item
elements rendered in the collapse (also present around the elements referenced
near the other block at lines showing the same pattern). Ensure the
aria-expanded logic and setMenuOpen remain unchanged while only altering
classes/styles for keyboard focus visibility.
Comment on lines
+112
to
124
| const handleTouchStart = (e: React.TouchEvent) => { | ||
| e.preventDefault(); | ||
| setIsVisible((prev) => !prev); | ||
| }; | ||
|
|
||
| return ( | ||
| <> | ||
| <div | ||
| ref={triggerRef} | ||
| onMouseEnter={handleMouseEnter} | ||
| onMouseLeave={handleMouseLeave} | ||
| onTouchStart={handleTouchStart} | ||
| className="inline-block" |
There was a problem hiding this comment.
Don’t cancel all touch defaults on the trigger.
preventDefault() here blocks the follow-up click on touch devices, so wrapped interactive children like links or buttons can stop working. Toggle the tooltip without cancelling the touch event.
Suggested fix
- const handleTouchStart = (e: React.TouchEvent) => {
- e.preventDefault();
+ const handleTouchStart = () => {
setIsVisible((prev) => !prev);
};📝 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
| const handleTouchStart = (e: React.TouchEvent) => { | |
| e.preventDefault(); | |
| setIsVisible((prev) => !prev); | |
| }; | |
| return ( | |
| <> | |
| <div | |
| ref={triggerRef} | |
| onMouseEnter={handleMouseEnter} | |
| onMouseLeave={handleMouseLeave} | |
| onTouchStart={handleTouchStart} | |
| className="inline-block" | |
| const handleTouchStart = () => { | |
| setIsVisible((prev) => !prev); | |
| }; | |
| return ( | |
| <> | |
| <div | |
| ref={triggerRef} | |
| onMouseEnter={handleMouseEnter} | |
| onMouseLeave={handleMouseLeave} | |
| onTouchStart={handleTouchStart} | |
| className="inline-block" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/powernight/web/src/components/Tooltip.tsx` around lines 112 - 124, The
touch handler handleTouchStart currently calls e.preventDefault(), which blocks
follow-up clicks on touch devices and breaks interactive children; remove the
e.preventDefault() call (or conditionally skip it for interactive elements) and
simply toggle setIsVisible((prev) => !prev) so the tooltip still opens on touch
without cancelling the native click for links/buttons wrapped by triggerRef.
Comment on lines
+345
to
+356
| async createTaskPreset(preset: { | ||
| name: string; | ||
| command: CommandType; | ||
| command_params: Record<string, any>; | ||
| default_time?: string; | ||
| }): Promise<TaskPreset> { | ||
| const response = await this.client.post<ApiResponse<TaskPreset>>('/tasks/presets', preset); | ||
| if (!response.data.success) { | ||
| throw new Error(response.data.error || 'Failed to create preset'); | ||
| } | ||
| return response.data.data!; | ||
| } |
There was a problem hiding this comment.
Missing CommandType import causes TypeScript error.
The createTaskPreset method uses CommandType in its parameter type at line 347, but CommandType is not imported from '../types'. This will fail type-checking.
🐛 Proposed fix
import {
ApiResponse,
SystemStatus,
BackupReserveData,
ScheduleEntry,
HealthStatus,
Metrics,
ActivityEntry,
NotificationConfig,
ReserveFormData,
ScheduleFormData,
Task,
TaskFormData,
TaskExecution,
TaskExecutionLogsResponse,
CommandDefinition,
- TaskPreset
+ TaskPreset,
+ CommandType
} from '../types';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/powernight/web/src/utils/api.ts` around lines 345 - 356, The
createTaskPreset method references the CommandType type but it isn't imported,
causing a TypeScript error; add an import for CommandType from the module that
exports your types (e.g., import { CommandType } from '../types') at the top of
the file so the type annotation on createTaskPreset is resolved; ensure the
import is included alongside other imports in
src/powernight/web/src/utils/api.ts and that the symbol name exactly matches
CommandType used in the function signature.
There was a problem hiding this comment.
Pull request overview
Adds a “Task Preset” feature spanning database, backend API, and Planner UI to support reusable predefined task configurations, alongside several responsive UX refinements and planning docs updates.
Changes:
- Introduces
TaskPresetpersistence (model/service) and seeds built-in presets during migration. - Adds REST endpoints for listing/creating/deleting presets and wires them into the React Planner UI.
- Improves mobile/responsive UX across Header/Planner/Dashboard/LogsTable and updates Tooltip touch behavior; updates frontend dependencies.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/powernight/web/src/utils/api.ts | Adds typed client methods for task preset endpoints. |
| src/powernight/web/src/types/index.ts | Introduces TaskPreset TypeScript type. |
| src/powernight/web/src/pages/Settings.tsx | Improves responsive form layout for inputs/buttons. |
| src/powernight/web/src/pages/Planner.tsx | Adds preset loading/selection, “save as preset” modal, and responsive table tweaks. |
| src/powernight/web/src/pages/Dashboard.tsx | Makes section header layout wrap better on smaller screens. |
| src/powernight/web/src/components/Tooltip.tsx | Adds touch toggle + outside-tap dismissal behavior. |
| src/powernight/web/src/components/LogsTable.tsx | Adds horizontal scrolling container + min column widths for mobile. |
| src/powernight/web/src/components/Header.tsx | Adds mobile hamburger menu and hides desktop nav on small screens. |
| src/powernight/web/api/tasks_api.py | Adds /tasks/presets GET/POST and /tasks/presets/<id> DELETE endpoints. |
| src/powernight/core/database/services.py | Adds TaskPresetService for CRUD operations. |
| src/powernight/core/database/models.py | Adds TaskPreset SQLAlchemy model + serialization. |
| src/powernight/core/database/migration.py | Seeds built-in presets during migration run. |
| src/powernight/core/database/exceptions.py | Adds PresetNotFoundError. |
| src/powernight/core/database/init.py | Exports TaskPreset and PresetNotFoundError. |
| package.json | Updates frontend dependencies/tooling versions. |
| docs/plans/ux-improvement-roadmap.md | Adds UX roadmap planning document. |
| docs/plans/2026-04-25-security-remediation.md | Adds security remediation planning document. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+438
to
+450
| {selectedPresetId && (() => { | ||
| const selected = presets.find(p => p.id === selectedPresetId); | ||
| return selected && !selected.is_builtin ? ( | ||
| <button | ||
| type="button" | ||
| onClick={() => handleDeletePreset(selectedPresetId)} | ||
| className="px-3 py-2 text-red-600 border border-red-300 rounded-md hover:bg-red-50" | ||
| title="Delete this preset" | ||
| > | ||
| Delete | ||
| </button> | ||
| ) : null; | ||
| })()} |
There was a problem hiding this comment.
Using an IIFE inside JSX ({selectedPresetId && (() => { ... })()}) makes this render path harder to read/debug and is easy to accidentally break with hooks/state. Consider computing selectedPreset once (e.g., via a local const or useMemo) and rendering conditionally without an inline function call.
Suggested change
| {selectedPresetId && (() => { | |
| const selected = presets.find(p => p.id === selectedPresetId); | |
| return selected && !selected.is_builtin ? ( | |
| <button | |
| type="button" | |
| onClick={() => handleDeletePreset(selectedPresetId)} | |
| className="px-3 py-2 text-red-600 border border-red-300 rounded-md hover:bg-red-50" | |
| title="Delete this preset" | |
| > | |
| Delete | |
| </button> | |
| ) : null; | |
| })()} | |
| {selectedPresetId && presets.find(p => p.id === selectedPresetId)?.is_builtin === false && ( | |
| <button | |
| type="button" | |
| onClick={() => handleDeletePreset(selectedPresetId)} | |
| className="px-3 py-2 text-red-600 border border-red-300 rounded-md hover:bg-red-50" | |
| title="Delete this preset" | |
| > | |
| Delete | |
| </button> | |
| )} |
Comment on lines
+708
to
+715
| if not request.is_json: | ||
| raise ValidationError("Request must contain JSON data") | ||
|
|
||
| data = request.get_json() | ||
|
|
||
| # Validate required fields | ||
| if not data.get('name', '').strip(): | ||
| raise ValidationError("Preset name is required") |
There was a problem hiding this comment.
request.get_json() can return None (or raise on invalid JSON) even when request.is_json is true. Consider using request.get_json(silent=True) and explicitly validating data is a dict before accessing .get(...) to avoid returning a 500 for malformed bodies.
Comment on lines
+679
to
+687
| <div className="fixed inset-0 bg-black bg-opacity-50 flex items-center justify-center z-50"> | ||
| <div className="bg-white p-6 rounded-lg shadow-xl max-w-md w-full mx-4"> | ||
| <h3 className="text-lg font-semibold mb-4">Save as Preset</h3> | ||
| <div className="space-y-4"> | ||
| <div> | ||
| <label className="block text-sm font-medium text-gray-700 mb-1"> | ||
| Preset Name | ||
| </label> | ||
| <input |
There was a problem hiding this comment.
The new "Save as Preset" modal is implemented inline here, duplicating modal markup/behavior (overlay, close handling, keyboard behavior) and lacks basic dialog semantics (role="dialog", aria-modal, labelled-by). Consider reusing a shared modal component (or introducing one) to ensure consistent accessibility and interaction patterns.
Suggested change
| <div className="fixed inset-0 bg-black bg-opacity-50 flex items-center justify-center z-50"> | |
| <div className="bg-white p-6 rounded-lg shadow-xl max-w-md w-full mx-4"> | |
| <h3 className="text-lg font-semibold mb-4">Save as Preset</h3> | |
| <div className="space-y-4"> | |
| <div> | |
| <label className="block text-sm font-medium text-gray-700 mb-1"> | |
| Preset Name | |
| </label> | |
| <input | |
| <div | |
| className="fixed inset-0 bg-black bg-opacity-50 flex items-center justify-center z-50" | |
| onClick={(e) => { | |
| if (e.target === e.currentTarget) { | |
| setShowSavePresetModal(false); | |
| } | |
| }} | |
| > | |
| <div | |
| className="bg-white p-6 rounded-lg shadow-xl max-w-md w-full mx-4" | |
| role="dialog" | |
| aria-modal="true" | |
| aria-labelledby="save-preset-modal-title" | |
| onKeyDown={(e) => { | |
| if (e.key === 'Escape') { | |
| setShowSavePresetModal(false); | |
| } | |
| }} | |
| > | |
| <h3 id="save-preset-modal-title" className="text-lg font-semibold mb-4">Save as Preset</h3> | |
| <div className="space-y-4"> | |
| <div> | |
| <label htmlFor="preset-name-input" className="block text-sm font-medium text-gray-700 mb-1"> | |
| Preset Name | |
| </label> | |
| <input | |
| id="preset-name-input" |
Comment on lines
+727
to
+733
| # Validate command parameters if provided | ||
| command_params = data.get('command_params', {}) | ||
| if command_params: | ||
| command = CronCommand(data['command'], command_params) | ||
| validation = command.validate() | ||
| if not validation.valid: | ||
| raise ValidationError(f"Invalid command parameters: {', '.join(validation.errors)}") |
There was a problem hiding this comment.
command_params is assumed to be an object/dict, but a non-dict JSON value (string/array) will cause CronCommand(...).validate() to throw and be reported as a 500. Add a type check (e.g., ensure isinstance(command_params, dict)) and return a 422 Validation Error when it's not a mapping.
Comment on lines
+693
to
+700
| except Exception as e: | ||
| current_app.logger.error(f"Failed to list presets: {e}") | ||
| return jsonify({ | ||
| 'success': False, | ||
| 'error': 'Internal Server Error', | ||
| 'message': f'Failed to list presets: {e}', | ||
| 'timestamp': datetime.now(timezone.utc).isoformat() | ||
| }), 500 |
There was a problem hiding this comment.
These error responses include the raw exception text ({e}) in the JSON payload. This can leak internal details to clients; log the exception server-side, but return a generic public message in the response body.
Comment on lines
+770
to
+777
| except Exception as e: | ||
| current_app.logger.error(f"Failed to create preset: {e}") | ||
| return jsonify({ | ||
| 'success': False, | ||
| 'error': 'Internal Server Error', | ||
| 'message': f'Failed to create preset: {e}', | ||
| 'timestamp': datetime.now(timezone.utc).isoformat() | ||
| }), 500 |
There was a problem hiding this comment.
The 500 handler returns message: f'Failed to create preset: {e}', exposing internal exception details to the client. Prefer returning a generic message (and optionally a request ID) while logging the full exception server-side.
Comment on lines
+802
to
+809
| except DatabaseError as e: | ||
| if "Built-in" in str(e): | ||
| return jsonify({ | ||
| 'success': False, | ||
| 'error': 'Forbidden', | ||
| 'message': str(e), | ||
| 'timestamp': datetime.now(timezone.utc).isoformat() | ||
| }), 403 |
There was a problem hiding this comment.
Detecting the "built-in preset" case via substring matching on str(e) is brittle (message changes will break behavior). Prefer raising/catching a dedicated exception for forbidden built-in deletion (or an explicit error code) and mapping that to a 403 here.
Comment on lines
+112
to
+113
| const handleTouchStart = (e: React.TouchEvent) => { | ||
| e.preventDefault(); |
There was a problem hiding this comment.
Calling preventDefault() on touchstart can block native scrolling and other default touch interactions on mobile when the user starts a scroll gesture on a tooltip trigger. Consider removing preventDefault (or only preventing default for specific non-scroll cases) and using a click/pointer handler to toggle visibility.
Suggested change
| const handleTouchStart = (e: React.TouchEvent) => { | |
| e.preventDefault(); | |
| const handleTouchStart = () => { |
| <div className="flex items-center md:hidden"> | ||
| <button | ||
| onClick={() => setMenuOpen(!menuOpen)} | ||
| className="text-white p-2 rounded-md hover:bg-white/10 focus:outline-none" |
There was a problem hiding this comment.
The mobile menu button removes the default focus outline (focus:outline-none) but doesn't add a replacement focus style. Add a visible focus indicator (e.g., focus-visible:ring-*) so keyboard users can see focus location.
Suggested change
| className="text-white p-2 rounded-md hover:bg-white/10 focus:outline-none" | |
| className="text-white p-2 rounded-md hover:bg-white/10 focus:outline-none focus-visible:ring-2 focus-visible:ring-white focus-visible:ring-offset-2 focus-visible:ring-offset-blue-600" |
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
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.
Summary
Test plan
/api/v1/tasks/presetsreturns seeded preset datapytest tests/to confirm no backend regressionsnpm run testandnpm run type-checkfor frontend checksSummary by CodeRabbit
New Features
Documentation
Chores