Planning
This commit is contained in:
34
notes/error-handling-discussion.md
Normal file
34
notes/error-handling-discussion.md
Normal file
@@ -0,0 +1,34 @@
|
||||
# Error Handling Design Discussion - Paused
|
||||
|
||||
## Context
|
||||
Exploring a comprehensive Rust error handling strategy with these criteria:
|
||||
1. Fatal errors propagate to top for clean shutdown (no panics—safety-critical environment)
|
||||
2. Normal errors handled close to source
|
||||
3. Adding enum variants forces compile errors on incomplete matches
|
||||
4. Function return types should be precise about possible errors
|
||||
|
||||
## Conclusion So Far
|
||||
**`Result<Result<T, Error>, Fatal>` remains the best option** for these requirements.
|
||||
|
||||
The nested Result pattern:
|
||||
- Satisfies all four criteria
|
||||
- Makes fatal errors ergonomic via `?` propagation
|
||||
- Forces explicit handling of recoverable errors
|
||||
- Is unconventional but justified for safety-critical contexts
|
||||
|
||||
## Rejected Alternatives
|
||||
- **Panic for fatal**: Ruled out—need explicit control for clean shutdown, data integrity
|
||||
- **Big enum**: Violates precision and exhaustive matching requirements
|
||||
- **Prior `result-extended/` crate**: Syntax sugar was awkward in practice
|
||||
|
||||
## Open Questions / Future Work
|
||||
- Could batch fatal-prone operations early in functions to simplify logic flow
|
||||
- Ergonomic helpers (macros, traits) might reduce verbosity but haven't found a clean approach yet
|
||||
- The `visions/core/src/app.rs` code has repetitive patterns (especially `database.open()`) that could potentially be streamlined
|
||||
|
||||
## Key Insight
|
||||
The pattern makes fatal errors ergonomic at the cost of recoverable error verbosity—this tradeoff is acceptable given the safety requirements.
|
||||
|
||||
## Reference Files
|
||||
- `visions/core/src/app.rs` - Current implementation using nested Result
|
||||
- `result-extended/` - Prior attempt at syntax sugar (found awkward)
|
||||
26
planning/01 user stories/US0038 App refactoring.md
Normal file
26
planning/01 user stories/US0038 App refactoring.md
Normal file
@@ -0,0 +1,26 @@
|
||||
---
|
||||
id: US0000
|
||||
type: userstory
|
||||
status: Not Started
|
||||
---
|
||||
|
||||
## Story
|
||||
As the developer, I want to refactor the App component to reduce coupling and code duplication
|
||||
|
||||
## Acceptance Criteria
|
||||
- AC1
|
||||
- AC2
|
||||
- AC3
|
||||
|
||||
## Linked Tasks
|
||||
|
||||
|
||||
## Planning
|
||||
- [[US0038-planning]]
|
||||
|
||||
## Manual Tests (links)
|
||||
- [[M000-login-success]]
|
||||
|
||||
## Status
|
||||
- Status: Not Started / In Progress / Blocked / Done
|
||||
- Remaining tasks: 0 / N <!-- keep updated or automate -->
|
||||
@@ -0,0 +1,28 @@
|
||||
---
|
||||
id: US0039
|
||||
type: userstory
|
||||
status: Not Started
|
||||
---
|
||||
|
||||
## Story
|
||||
**As a** [role]
|
||||
**I want** [goal]
|
||||
**So that** [benefit]
|
||||
|
||||
## Acceptance Criteria
|
||||
- AC1
|
||||
- AC2
|
||||
- AC3
|
||||
|
||||
## Linked Tasks
|
||||
- [[T000-task-example]] <!-- use Obsidian wikilinks -->
|
||||
|
||||
## Planning
|
||||
- [[US0000]] <!-- link to planning doc (see planning template) -->
|
||||
|
||||
## Manual Tests (links)
|
||||
- [[M000-login-success]]
|
||||
|
||||
## Status
|
||||
- Status: Not Started / In Progress / Blocked / Done
|
||||
- Remaining tasks: 0 / N <!-- keep updated or automate -->
|
||||
@@ -0,0 +1,28 @@
|
||||
---
|
||||
id: US0040
|
||||
type: userstory
|
||||
status: Not Started
|
||||
---
|
||||
|
||||
## Story
|
||||
**As a** [role]
|
||||
**I want** [goal]
|
||||
**So that** [benefit]
|
||||
|
||||
## Acceptance Criteria
|
||||
- AC1
|
||||
- AC2
|
||||
- AC3
|
||||
|
||||
## Linked Tasks
|
||||
- [[T000-task-example]] <!-- use Obsidian wikilinks -->
|
||||
|
||||
## Planning
|
||||
- [[US0000]] <!-- link to planning doc (see planning template) -->
|
||||
|
||||
## Manual Tests (links)
|
||||
- [[M000-login-success]]
|
||||
|
||||
## Status
|
||||
- Status: Not Started / In Progress / Blocked / Done
|
||||
- Remaining tasks: 0 / N <!-- keep updated or automate -->
|
||||
@@ -0,0 +1,28 @@
|
||||
---
|
||||
id: US0041
|
||||
type: userstory
|
||||
status: Not Started
|
||||
---
|
||||
|
||||
## Story
|
||||
**As a** [role]
|
||||
**I want** [goal]
|
||||
**So that** [benefit]
|
||||
|
||||
## Acceptance Criteria
|
||||
- AC1
|
||||
- AC2
|
||||
- AC3
|
||||
|
||||
## Linked Tasks
|
||||
- [[T000-task-example]] <!-- use Obsidian wikilinks -->
|
||||
|
||||
## Planning
|
||||
- [[US0000]] <!-- link to planning doc (see planning template) -->
|
||||
|
||||
## Manual Tests (links)
|
||||
- [[M000-login-success]]
|
||||
|
||||
## Status
|
||||
- Status: Not Started / In Progress / Blocked / Done
|
||||
- Remaining tasks: 0 / N <!-- keep updated or automate -->
|
||||
222
planning/04 planning/US0038-planning.md
Normal file
222
planning/04 planning/US0038-planning.md
Normal file
@@ -0,0 +1,222 @@
|
||||
# Planning / US000 — Detailed plan
|
||||
|
||||
## Overview
|
||||
Brief summary of approach.
|
||||
|
||||
## Architecture / Design notes
|
||||
- components, models, API sketches
|
||||
|
||||
## Implementation plan (steps)
|
||||
1. Step 1
|
||||
2. Step 2
|
||||
3. Step 3
|
||||
|
||||
## Risks & Dependencies
|
||||
- external systems, third-party libs, design decisions
|
||||
|
||||
## Estimation
|
||||
- Rough estimate (hours / story points)
|
||||
|
||||
## Acceptance / QA notes
|
||||
- Anything QA must verify beyond ACs.
|
||||
|
||||
## Open questions
|
||||
- Q1, Q2
|
||||
|
||||
## AI suggestions
|
||||
### Database Access Boilerplate
|
||||
|
||||
The most pervasive duplication is the database access pattern, repeated ~40 times:
|
||||
|
||||
```rust
|
||||
let state = self.inner.read().await; // or write()
|
||||
let db = match state.database.open() {
|
||||
Ok(db) => db,
|
||||
Err(_) => return Ok(Err(Error::CannotOpen)),
|
||||
};
|
||||
```
|
||||
|
||||
Suggested refactoring: Create helper methods that encapsulate this:
|
||||
|
||||
```rust
|
||||
impl App {
|
||||
async fn with_db<F, T>(&self, f: F) -> Result<Result<T, Error>, Fatal>
|
||||
where
|
||||
F: FnOnce(&DbHandle) -> Result<Result<T, Error>, Fatal>,
|
||||
{
|
||||
let state = self.inner.read().await;
|
||||
let db = state.database.open().map_err(|_| /* ... */)?;
|
||||
f(&db)
|
||||
}
|
||||
|
||||
async fn with_db_write<F, T>(&self, f: F) -> Result<Result<T, Error>, Fatal>
|
||||
where
|
||||
F: FnOnce(&mut RwLockWriteGuard<'_, App_>, &DbHandle) -> Result<Result<T, Error>, Fatal>,
|
||||
{ /* ... */ }
|
||||
}
|
||||
```
|
||||
|
||||
|
||||
### Authorization Checks Mixed with Business Logic
|
||||
|
||||
Authorization is scattered throughout functions. I see three patterns:
|
||||
|
||||
Pattern A - GM-only operations (lines 814, 841, 867, 893, 941, 1017, 1044, 1081, 1133, 1166, 1211, 1250, 1294, 1348):
|
||||
```rust
|
||||
if game.gm != *user_id {
|
||||
return Ok(Err(Error::Forbidden));
|
||||
}
|
||||
// or
|
||||
if !user.admin && user.id != game.gm {
|
||||
return Ok(Err(Error::Forbidden));
|
||||
}
|
||||
```
|
||||
|
||||
Pattern B - Admin-only operations (lines 313, 330):
|
||||
```rust
|
||||
if user.admin {
|
||||
// proceed
|
||||
} else {
|
||||
Ok(Err(Error::Forbidden))
|
||||
}
|
||||
```
|
||||
|
||||
```rust
|
||||
Pattern C - Owner or GM (lines 1420, 1480):
|
||||
if *user_id == sheet.user_id || *user_id == game.gm {
|
||||
// proceed
|
||||
}
|
||||
```
|
||||
|
||||
Suggested refactoring: Create an authorization layer:
|
||||
|
||||
```rust
|
||||
enum Permission {
|
||||
Admin,
|
||||
GameGm(GameId),
|
||||
GameParticipant(GameId), // GM or player
|
||||
CharacterOwner(CharacterId),
|
||||
CharacterOwnerOrGm(CharacterId),
|
||||
}
|
||||
|
||||
impl App {
|
||||
async fn authorize(&self, user: &User, permission: Permission) -> Result<Result<(), Error>, Fatal> {
|
||||
// centralized authorization logic
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Or use a trait-based approach where operations declare their required permissions.
|
||||
|
||||
### Mixed Responsibilities in App
|
||||
|
||||
The App struct currently handles:
|
||||
Responsibility: WebAuthn management
|
||||
Lines: 387-570
|
||||
Methods: start_passkey_registration, finish_passkey_registration, start_passkey_auth, finish_passkey_auth
|
||||
────────────────────────────────────────
|
||||
Responsibility: Session management
|
||||
Lines: 257-605
|
||||
Methods: check_password, create_session, delete_session, user_from_session, touch_session
|
||||
────────────────────────────────────────
|
||||
Responsibility: User management
|
||||
Lines: 302-385, 607-630
|
||||
Methods: create_invitation, invitations, check_invitation, create_user, user, users
|
||||
────────────────────────────────────────
|
||||
Responsibility: Game management
|
||||
Lines: 632-848, 851-925
|
||||
Methods: game, games, create_game, link_player, unlink_player, open_to_spectators, close_to_spectators
|
||||
────────────────────────────────────────
|
||||
Responsibility: Tabletop/Scene management
|
||||
Lines: 927-1142
|
||||
Methods: set_tabletop_image, tabletop, scene, scenes, set_scene, create_scene, update_scene
|
||||
────────────────────────────────────────
|
||||
Responsibility: Image management
|
||||
Lines: 1144-1356
|
||||
Methods: link_image_to_scene, unlink_image_from_scene, add_image, delete_image, image, images
|
||||
────────────────────────────────────────
|
||||
Responsibility: Character management
|
||||
Lines: 1358-1497
|
||||
Methods: characters, charsheet, create_character, update_character
|
||||
────────────────────────────────────────
|
||||
Responsibility: Card management
|
||||
Lines: 1499-1635
|
||||
Methods: card, cards, cards_for_character, create_card, update_card
|
||||
────────────────────────────────────────
|
||||
Responsibility: WebSocket management
|
||||
Lines: 1637-2060
|
||||
Methods: register_websocket, get_socket, change_websocket_state, handle_request
|
||||
Suggested refactoring: Extract service layers:
|
||||
|
||||
```rust
|
||||
// Separate concerns into services
|
||||
struct AuthService { db: Database, authn: Webauthn }
|
||||
struct GameService { db: Database }
|
||||
struct SceneService { db: Database }
|
||||
struct ImageService { db: Database }
|
||||
struct CharacterService { db: Database }
|
||||
struct CardService { db: Database }
|
||||
|
||||
// App becomes a coordinator
|
||||
struct App {
|
||||
auth: AuthService,
|
||||
games: GameService,
|
||||
// ...
|
||||
connections: Arc<RwLock<ConnectionManager>>,
|
||||
}
|
||||
```
|
||||
|
||||
### Duplicate Game Lookup + Authorization
|
||||
|
||||
Many functions follow this exact sequence:
|
||||
7. Open database
|
||||
8. Lookup game by ID
|
||||
9. Check user is GM
|
||||
10. Perform operation
|
||||
|
||||
Examples: `link_player`, `unlink_player`, `open_to_spectators`, close_to_spectators, set_tabletop_image, set_scene, scenes, images, etc.
|
||||
|
||||
Suggested refactoring:
|
||||
```rust
|
||||
impl App {
|
||||
async fn game_as_gm(&self, user: &User, game_id: &GameId)
|
||||
-> Result<Result<(DbHandle, GameOverview), Error>, Fatal>
|
||||
{
|
||||
let state = self.inner.read().await;
|
||||
let db = state.database.open().map_err(/* ... */)?;
|
||||
let Some(game) = db.game(game_id)? else {
|
||||
return Ok(Err(Error::NotFound(game_id.to_string())));
|
||||
};
|
||||
if game.gm != user.id && !user.admin {
|
||||
return Ok(Err(Error::Forbidden));
|
||||
}
|
||||
Ok(Ok((db, game)))
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
|
||||
### Broadcast Logic Embedded in Business Methods
|
||||
|
||||
Methods like set_tabletop_image (line 927), update_character (line 1459), and update_card (line 1598) directly call broadcast_to_game or
|
||||
send_to_user. This couples business logic to the notification mechanism.
|
||||
|
||||
Suggested refactoring: Return events from business methods and let a higher layer handle broadcasting:
|
||||
|
||||
```rust
|
||||
enum AppEvent {
|
||||
TabletopChanged(GameId, Tabletop),
|
||||
CharacterUpdated(UserId, Charsheet),
|
||||
CardUpdated(GameId, Card),
|
||||
}
|
||||
|
||||
// Business method returns the event
|
||||
async fn set_tabletop_image(...) -> Result<Result<AppEvent, Error>, Fatal>
|
||||
|
||||
// Coordinator handles broadcasting
|
||||
match app.set_tabletop_image(...).await?? {
|
||||
AppEvent::TabletopChanged(game_id, tabletop) => {
|
||||
broadcast_to_game(&game_id, GameMessage::Tabletop(tabletop)).await;
|
||||
}
|
||||
}
|
||||
```
|
||||
@@ -0,0 +1,17 @@
|
||||
---
|
||||
id: D0056
|
||||
type: defect
|
||||
story:
|
||||
status: Not Started
|
||||
assignee:
|
||||
---
|
||||
|
||||
## Description
|
||||
Describe the task, implementation details.
|
||||
|
||||
## Definition of Done
|
||||
- DoD 1
|
||||
- DoD 2
|
||||
|
||||
## Notes
|
||||
- code paths, files to touch, tests to add
|
||||
Reference in New Issue
Block a user