Conversation
WalkthroughThis PR refactors application state into model/view/runtime/history, adds a snapshot-based, focus-aware History with undo/redo, and introduces a Redux-style reducer (Action/Effect) plus a centralized update pipeline that records actions to History and runs effects. It adds Message variants and hotkeys for Undo/Redo, moves many UI paths to app.model/app.view/app.runtime, and introduces state, reducer, history, and update modules along with associated tests. Sequence Diagram(s)sequenceDiagram
participant UI as User / UI
participant Msg as Message Handler
participant Reducer as Reducer
participant History as History
participant Effects as Effects Runner
UI->>Msg: User action (Edit / Send / Save)
Msg->>Reducer: reduce(model, Action)
Reducer-->>Msg: (updated_model, [Effects])
Msg->>History: record(Action, Focus, &model)
History-->>Msg: store entry / snapshot
Msg->>Effects: run_effects([Effects])
Effects-->>Msg: async results (e.g., ResponseReady)
Msg-->>UI: update view with updated_model / results
sequenceDiagram
participant UI as User / UI
participant Update as Update Handler
participant History as History
participant Reducer as Reducer
participant View as View / ApplyFocus
UI->>Update: Trigger Undo or Redo (Message)
Update->>History: undo(apply_focus) or redo(apply_focus)
History->>History: compute target cursor and nearest snapshot
History->>Reducer: replay actions from snapshot to target
loop apply actions
Reducer-->>History: (model, _effects)
end
History->>View: apply_focus(&Focus, &mut model)
View-->>Update: restored model + Focus (HistoryResult)
Update-->>UI: render restored model and focus
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
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: 0
🧹 Nitpick comments (5)
src/app/reducer.rs (1)
101-129: Consider logging or asserting on invalid header indices.While
HeaderIndex::newprevents invalid construction, these handlers silently ignore out-of-bounds access. If theheader_rowslist changes betweenHeaderIndexcreation and action dispatch, stale indices will be silently dropped.This is memory-safe but could hide bugs during development. Consider adding debug assertions or logging for invalid index cases to aid debugging.
Optional: Add debug assertion
Action::HeaderNameChanged(idx, value) => { - if let Some(row) = model.header_rows.get_mut(idx.get()) { + if let Some(row) = model.header_rows.get_mut(idx.get()) { row.name = value; model.rebuild_headers_from_rows(); + } else { + debug_assert!(false, "HeaderIndex out of bounds: {} >= {}", idx.get(), model.header_rows.len()); } (model, Vec::new()) }src/app/update.rs (3)
226-278: Minor edge case: stale collection index could cause silent failure.In the
Collectionbranch (line 233), if the collection index in the selection becomes stale (e.g., collections list is modified elsewhere),get_mut(collection)returnsNoneand the request silently fails to be added without user feedback.Consider adding status feedback for the
Nonecase, similar to the default branch.🔎 Proposed enhancement
Some(RequestId::Collection { collection, .. }) => { let new_draft = RequestDraft { title: "New request".to_string(), ..Default::default() }; if let Some(col) = self.view.collections.get_mut(collection) { col.requests.push(new_draft); let new_idx = col.requests.len() - 1; let new_id = RequestId::Collection { collection, index: new_idx, }; self.view.selection = Some(new_id.clone()); if let Some(loaded) = self.view.resolve_request(&new_id) { return self.apply_model_action(Action::LoadDraft(loaded), false, Some("Ready")); } + } else { + self.view.update_status_with_model("Collection not found", &self.model); } Task::none() }
293-309: Header check is case-sensitive, may miss lowercase variants.Line 303 uses
draft.headers.contains("Content-Type")which is case-sensitive. HTTP headers are case-insensitive, socontent-type: application/jsonwould not be detected, causing a duplicate header to be added.🔎 Proposed fix
- if !draft.headers.contains("Content-Type") { + if !draft.headers.to_lowercase().contains("content-type") { draft.headers.push_str("\nContent-Type: application/json"); }
348-348: UnnecessaryTask::none()in batch.
Task::batch([Task::none(), self.rescan_files()])includes a no-op task. This can be simplified.🔎 Proposed fix
- Task::batch([Task::none(), self.rescan_files()]) + self.rescan_files()src/app/lifecycle.rs (1)
111-117: Double space in window title - likely a typo.Line 113 has
"Zagel REST workbench"with two spaces between "Zagel" and "REST". This appears unintentional.🔎 Proposed fix
- .title("Zagel REST workbench") + .title("Zagel REST workbench")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/app/history.rssrc/app/hotkeys.rssrc/app/lifecycle.rssrc/app/messages.rssrc/app/mod.rssrc/app/reducer.rssrc/app/state.rssrc/app/update.rssrc/app/view/mod.rssrc/app/view/workspace.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runcargo clippy --all-targets --all-features -- -D warnings(orcargo clippy-strict) before committing to enforce the strict lint baseline
Make invalid state unrepresentable (embrace ADTs - Algebraic Data Types)
Parse, don't validate: parse input at the edges so downstream state stays valid and doesn't need repeated validation
Push ifs up, push fors (loops) down
Prefer immutability and functional programming style when you can without sacrificing code cleanliness
Files:
src/app/hotkeys.rssrc/app/reducer.rssrc/app/messages.rssrc/app/view/workspace.rssrc/app/view/mod.rssrc/app/mod.rssrc/app/update.rssrc/app/lifecycle.rssrc/app/history.rssrc/app/state.rs
🧬 Code graph analysis (6)
src/app/view/workspace.rs (2)
src/app/view/mod.rs (2)
section(23-34)view(36-60)src/app/view/response.rs (2)
response_tab_toggle(43-60)response_panel(71-146)
src/app/mod.rs (1)
src/app/lifecycle.rs (1)
run(111-117)
src/app/update.rs (6)
src/app/options.rs (2)
apply_auth_headers(79-105)build_graphql_body(69-77)src/app/reducer.rs (2)
reduce(62-136)new(14-20)src/app/history.rs (3)
new(63-74)record(76-92)from_selection(16-18)src/app/state.rs (4)
new(21-24)new(35-41)new(161-188)default(77-93)src/app/view/mod.rs (1)
view(36-60)src/net.rs (1)
send_request(8-80)
src/app/lifecycle.rs (4)
src/parser.rs (2)
scan_env_files(31-51)scan_http_files(11-29)src/app/view/mod.rs (1)
view(36-60)src/app/state.rs (4)
default(77-93)new(21-24)new(35-41)new(161-188)src/app/history.rs (1)
new(63-74)
src/app/history.rs (2)
src/app/reducer.rs (1)
reduce(62-136)src/model.rs (1)
from(46-55)
src/app/state.rs (3)
src/parser.rs (1)
suggest_http_path(167-194)src/app/status.rs (3)
default_environment(5-10)status_with_missing(19-35)with_default_environment(12-17)src/app/view/response.rs (1)
pretty_json(148-152)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and test (ubuntu-latest)
- GitHub Check: Build and test (windows-latest)
🔇 Additional comments (42)
src/app/reducer.rs (3)
10-25: Excellent type-safe design for HeaderIndex.The bounds-checked constructor and opaque wrapper prevent invalid indices from being constructed, following the "make invalid state unrepresentable" principle from the coding guidelines.
27-60: Well-structured Action/Effect separation.The Action and Effect enums provide clear separation between pure state transitions and side effects, enabling deterministic reducer logic and testable state transitions as outlined in the PR objectives.
89-96: No action needed. RequestDraft does not includegraphql_queryorgraphql_variablesfields (onlybody,headers,url,method, andtitle). The GraphQL editors are intentionally separate from the request draft and do not sync to it by design. The apparent inconsistency withBodyEditedis justified:BodyEditedsyncs becausebodyis a RequestDraft field, but GraphQL editors don't sync because they're not part of the draft structure.src/app/messages.rs (1)
43-44: LGTM! Undo/Redo message variants added.The new message variants cleanly integrate with the existing Message enum to support the undo/redo functionality.
src/app/hotkeys.rs (1)
13-23: LGTM! Undo/redo shortcuts correctly implemented.The pattern matching order is correct:
Cmd+Shift+Zis checked beforeCmd+Z, ensuring that shifted keypresses are properly distinguished. The addition ofCmd+Yas an alternative redo shortcut provides good cross-platform UX consistency.src/app/view/mod.rs (2)
39-52: LGTM! Field access updated for new view structure.The refactoring correctly updates all field access paths from the flat
app.*structure to the nestedapp.view.*structure, maintaining consistency with the new model-view-runtime separation.
62-77: LGTM! Status bar updated to use view state.The status bar correctly reads from
app.view.show_shortcutsandapp.view.status_line, maintaining consistency with the view state separation.src/app/mod.rs (1)
3-14: LGTM! Module organization updated for new architecture.The addition of
history,reducer, andstatemodules and the relocation ofHeaderRowtostatealign with the architectural refactoring. The public API surface remains compatible—HeaderRowis still exported, just from its new home in the state module.src/app/view/workspace.rs (5)
34-55: LGTM!The workspace function correctly accesses
app.view.workspace_panesfor the PaneGrid state, aligning with the model-view separation architecture.
70-184: LGTM!The
builder_formfunction correctly separates concerns:
- View state (
app.view) for environments and selection display- Model state (
app.model) for editable draft fields, headers, mode, and authThe
#[allow(clippy::too_many_lines)]is reasonable for this UI builder function.
186-225: LGTM!The
builder_bodyfunction correctly accessesapp.model.modeand the respective editor content fields from the model, maintaining the undoable state separation.
227-277: LGTM!The
responsefunction correctly accesses:
app.viewfor response display state, tabs, and shortcuts visibilityapp.runtime.state.themefor the highlight themeThis aligns well with the architecture where response preview is transient view state.
279-300: LGTM!The shortcuts panel correctly displays the new undo/redo hotkeys, matching the implementation in hotkeys.rs.
src/app/update.rs (7)
17-53: LGTM!The basic message handlers correctly:
- Use
SplitRatio::newto clamp pane resize ratios- Update view state for non-recordable changes
- Save runtime state after environment changes
55-99: LGTM!The model action handlers follow a consistent pattern with proper validation:
HeaderIndex::newvalidates indices before creating actions- User edits are marked
record: truefor undo/redo support- Status base is appropriately set for feedback
153-178: LGTM!The
apply_model_actionfunction correctly:
- Uses
std::mem::taketo move model ownership to the reducer- Records actions with focus derived from selection
- Trims history after recording with the appropriate focus applier
The borrow pattern with
let view = &self.viewbefore callingself.history.trim_if_neededis correct since the closure only borrowsviewimmutably.
180-213: LGTM!The
run_effectsfunction cleanly mapsEffectvariants to concrete tasks, usingTask::batchto combine multiple effects. The early return for empty effects avoids unnecessary allocations.
216-224: LGTM!The
handle_selectionfunction correctly:
- Sets the selection in view state (non-recordable)
- Loads the draft via action without recording (selection changes are view-only per PR objectives)
- Updates the response viewer after selection change
358-384: LGTM!The
handle_undoandhandle_redofunctions correctly:
- Call history methods with the focus applier closure
- Restore model state and selection from the history result
- Update view state (status, response viewer) after restoration
388-395: LGTM!The
apply_focus_with_viewhelper correctly bridges the Focus and ViewState to apply the appropriate draft to the model during history replay.src/app/history.rs (8)
9-33: LGTM!The
Focusenum provides a clean abstraction for tracking request context with useful conversion methods between selection and focus representations.
35-52: LGTM!The internal data structures are well-designed:
HistoryEntrycaptures action and focus for replaySnapshotstores full model state at checkpointsHistoryResultprovides the public interface for undo/redo results
62-74: LGTM!The
History::newfunction correctly initializes the history with a baseline snapshot at index 0, allowing replay from the initial state.
76-92: LGTM!The
recordfunction correctly:
- Truncates future history when recording after an undo
- Retains snapshots up to the current cursor position
- Creates periodic snapshots at
SNAPSHOT_INTERVALboundaries
94-114: LGTM!The
undoandredofunctions have correct boundary checks and replay to the appropriate cursor position after adjustment.
116-134: LGTM!The
trim_if_neededfunction correctly manages history size by:
- Replaying to the overflow point to establish the new base state
- Draining old entries and adjusting the cursor with
saturating_sub- Creating a fresh snapshot representing the new baseline
136-164: Consider the edge case wheretargetequalssnapshot.index.When
target == snapshot.index, the loopself.entries.iter().take(target).skip(snapshot.index)produces an empty iterator, solast_focusremainssnapshot.focus.clone()without ever being updated by the loop body. This is likely correct behavior, but worth noting.Verify this edge case is handled correctly: when target equals the snapshot index, the returned focus should be the snapshot's focus, not
Focus::None.
167-251: LGTM!The test
history_replay_respects_focusprovides good coverage for the core undo/redo behavior with focus switching between different requests, verifying both the model state and focus are correctly restored.src/app/lifecycle.rs (4)
18-23: LGTM!The refactored
Zagelstruct cleanly separates concerns with the model-view-runtime-history composition, with appropriatepub(super)visibility.
26-84: LGTM!The
initfunction correctly initializes all components in the right order:
- Creates pane layouts with sensible default ratios
- Initializes
AppModelwith defaults- Creates
ViewStatewith the root path and pane states- Assembles
Runtimewith client and persisted state- Initializes
Historywith a clone of the model for the baseline snapshot
86-95: LGTM!The
subscriptionandthemefunctions correctly access the refactored state structure, withthemeremaining aconst fn.
97-108: LGTM!The
rescan_filesfunction correctly usesself.view.http_rootas the scan root and batches both file scans for concurrent execution.src/app/state.rs (10)
15-29: LGTM!
SplitRatiocorrectly clamps values to[0.2, 0.8]to prevent extreme pane splits, following the "make invalid state unrepresentable" principle.
31-50: LGTM!
EnvironmentIndexprovides safe bounds-checked access to environments, following the "make invalid state unrepresentable" principle. Thefindmethod handles missing environments gracefully.
52-62: LGTM!
HeaderRowandLoadedDraftare clean data structures supporting header editing and draft loading workflows.
64-94: LGTM!
AppModeland itsDefaultimplementation correctly initialize:
- Editor content synchronized with draft
- GraphQL variables with valid empty JSON
{}- A single empty header row for immediate editing
96-138: LGTM!The
AppModelmethods correctly handle bidirectional header conversion:
split_once(':')properly handles header values containing colons- Empty headers result in a single empty row for editing
rebuild_headers_from_rowsfilters out empty-name rows
140-158: LGTM!
ViewStateappropriately groups all UI-related state that should not be part of undo/redo history, including pane states, response display, and environment selection.
160-188: LGTM!
ViewState::newcorrectly initializes all fields with sensible defaults. Theexpecton line 169 is safe because the environments vector is guaranteed to have the default environment.
190-210: LGTM!The status update methods correctly compute the status line, including GraphQL query and variables for missing variable detection when in GraphQL mode.
212-248: LGTM!The methods correctly handle:
- Response formatting with pretty JSON when applicable
- Environment list management with default fallback
- Safe environment name access through validated
EnvironmentIndex
250-280: LGTM!
resolve_requestcorrectly resolves drafts from bothCollectionandHttpFilesources, with appropriate save path derivation:
HttpFile: uses the existing file pathCollection: generates a suggested path usingsuggest_http_path
Runtimecleanly encapsulates the HTTP client and persisted application state.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/app/history.rs (1)
116-134: Consider documenting thattrim_if_neededrebuilds the base snapshot.The method correctly handles overflow by replaying to the overflow point, draining old entries, and resetting snapshots. The
saturating_subon line 127 safely handles the cursor adjustment. This is a well-implemented trimming strategy.One minor note: the method signature requires
apply_focuseven though it's only used internally for replay. This is consistent with the API, but a brief doc comment explaining the trim behavior would help future maintainers.src/app/state.rs (2)
112-135: Header parsing may silently drop malformed lines.In
set_header_rows_from_draft(), lines without a:separator are silently skipped (line 122-127). While the fallback to an empty row at lines 129-134 ensures at least one row exists, malformed header lines in the original draft are lost without warning.This may be intentional for robustness, but consider whether logging or preserving malformed lines would be preferable for debugging purposes.
205-214: Potential inefficiency:text()clones the entire editor content.The calls to
model.graphql_query.text()andmodel.graphql_variables.text()on lines 208 allocate newStringinstances. For status updates that may happen frequently (e.g., on every keystroke), this could add allocation pressure.Consider whether the status needs to be updated on every model change, or if it could be debounced/throttled at a higher level.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/app/history.rssrc/app/hotkeys.rssrc/app/lifecycle.rssrc/app/messages.rssrc/app/mod.rssrc/app/reducer.rssrc/app/state.rssrc/app/update.rssrc/app/view/mod.rssrc/app/view/workspace.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/reducer.rs
- src/app/view/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runcargo clippy --all-targets --all-features -- -D warnings(orcargo clippy-strict) before committing to enforce the strict lint baseline
Make invalid state unrepresentable (embrace ADTs - Algebraic Data Types)
Parse, don't validate: parse input at the edges so downstream state stays valid and doesn't need repeated validation
Push ifs up, push fors (loops) down
Prefer immutability and functional programming style when you can without sacrificing code cleanliness
Files:
src/app/messages.rssrc/app/hotkeys.rssrc/app/update.rssrc/app/view/workspace.rssrc/app/lifecycle.rssrc/app/state.rssrc/app/mod.rssrc/app/history.rs
🧬 Code graph analysis (4)
src/app/update.rs (5)
src/app/options.rs (2)
apply_auth_headers(79-105)build_graphql_body(69-77)src/app/reducer.rs (2)
reduce(61-135)new(14-20)src/app/state.rs (4)
new(22-25)new(36-42)new(172-203)default(85-101)src/app/history.rs (2)
new(63-74)from_selection(16-18)src/model.rs (3)
error(106-114)default(68-76)from(46-55)
src/app/state.rs (6)
src/app/status.rs (3)
default_environment(5-10)status_with_missing(19-35)with_default_environment(12-17)src/app/view/mod.rs (1)
view(36-61)src/app/reducer.rs (2)
new(14-20)get(22-24)src/app/history.rs (1)
new(63-74)src/model.rs (1)
as_str(27-36)src/app/view/response.rs (1)
pretty_json(148-152)
src/app/mod.rs (3)
src/app/update.rs (1)
update(87-484)src/app/view/mod.rs (1)
view(36-61)src/app/lifecycle.rs (1)
run(116-122)
src/app/history.rs (3)
src/app/reducer.rs (2)
reduce(61-135)new(14-20)src/app/state.rs (4)
new(22-25)new(36-42)new(172-203)default(85-101)src/model.rs (1)
from(46-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and test (windows-latest)
- GitHub Check: Build and test (ubuntu-latest)
- GitHub Check: Build and test (windows-latest)
🔇 Additional comments (30)
src/app/messages.rs (1)
57-58: LGTM! Clean addition of Undo/Redo message variants.The new
UndoandRedomessage variants are well-positioned in the enum and align with the PR's focus on implementing undo/redo functionality through an event-sourced reducer design.src/app/hotkeys.rs (1)
13-23: LGTM! Hotkey ordering is correct.The undo/redo hotkeys are properly implemented with correct precedence:
- Cmd+Shift+Z (line 14) is checked before Cmd+Z (line 18), ensuring the more specific modifier combination takes priority
- Cmd+Y (line 21) provides standard Windows-style redo
- Case-insensitive matching ensures both 'Z'/'z' and 'Y'/'y' work
This aligns well with the PR's focus-aware undo/redo system.
src/app/mod.rs (1)
3-16: LGTM! Clean module organization supporting the new architecture.The additions of
history,reducer, andstatemodules along with the reorganized re-exports properly establish the separation between model/view/runtime/history as outlined in the PR objectives. MovingEditStateandHeaderRowfromlifecycletostateimproves cohesion.src/app/view/workspace.rs (6)
35-79: LGTM! Consistent refactoring to view state paths.The workspace and builder pane access, along with environment fields, have been properly migrated to use
app.view.*paths, aligning with the new model/view/runtime architecture.
90-132: LGTM! Draft and mode fields correctly migrated to model.The method, URL, title, save path, and mode fields are now accessed via
app.model.draft.*andapp.model.mode, which correctly places undoable application state in the model layer.
139-171: LGTM! Auth and header fields properly moved to model.Authentication state and header rows are now accessed via
app.model.authandapp.model.header_rows, maintaining consistency with the undoable state architecture.
187-217: LGTM! Body editor fields correctly placed in model.The GraphQL query/variables and body editor are now accessed via
app.model.*, ensuring these undoable editing states are properly managed through the reducer pipeline.
229-244: LGTM! Response view fields correctly separated.Response display state is accessed via
app.view.*(non-undoable UI state) while the theme comes fromapp.runtime.state.theme, demonstrating proper separation of concerns.
288-290: LGTM! Shortcuts documentation updated for undo/redo.The shortcuts panel now properly documents the new undo/redo hotkeys (Ctrl/Cmd+Z, Ctrl/Cmd+Shift+Z, Ctrl/Cmd+Y), providing clear user guidance.
src/app/update.rs (9)
13-17: LGTM! Imports properly support the new architecture.The new imports for
Focus,Action,Effect,reduce,AppModel,ViewState, andSplitRatioalign with the PR's event-sourced reducer design and model/view/runtime separation.
387-432: LGTM! Message handlers correctly converted to action/effect flow.The handlers for
MethodSelected,UrlChanged,TitleChanged,ModeChanged, editor actions, and header operations have been properly converted to useapply_model_action, with appropriaterecordflags and status messages. TheHeaderIndex::new()validation (lines 413, 419, 428) guards against out-of-bounds access before creating actions.
486-512: LGTM! Action/effect pipeline correctly implemented.The
apply_model_actionmethod properly:
- Captures focus from the current selection before applying the action (lines 492-496)
- Uses
std::mem::taketo move the model without cloning (line 497)- Records the action in history with the captured focus (line 501)
- Trims history using the
apply_focus_with_viewclosure to maintain bounded history (lines 503-504)- Updates status and executes effects (lines 507-511)
This aligns with the PR's event-sourced reducer design.
514-546: LGTM! Effect execution properly centralized.The
run_effectsmethod correctly:
- Early returns for empty effect lists (lines 515-517)
- Maps each effect variant to its corresponding task (lines 519-543)
- Uses
Task::batchto execute all effects in parallel (line 545)This centralized execution point simplifies effect handling and aligns with the reducer pattern.
659-677: File rescan after save ensures consistency.Line 669 correctly triggers a file rescan after successful save, which reconciles the in-memory
http_filesstate with the persisted disk state. This is necessary given that persistence happens asynchronously through effects.
614-630: LGTM! Request preparation correctly handles GraphQL and auth.The
prepare_sendmethod properly:
- Converts GraphQL requests to POST with a JSON body (lines 617-626)
- Applies authentication headers (line 628)
- Tracks extra inputs (query/variables) for status display (lines 621-622)
679-705: LGTM! Undo/redo correctly integrated with history and focus.Both
handle_undoandhandle_redoproperly:
- Invoke the history system with the
apply_focus_with_viewclosure (lines 682-683, 696-697)- Update
self.modelwith the replayed state (lines 685, 699)- Synchronize
self.view.selectionfrom the focus (lines 686, 700)- Update status and response viewer to reflect the new state (lines 687-688, 701-702)
This implements the focus-aware undo/redo specified in the PR objectives.
708-715: LGTM! Focus application correctly loads draft.The
apply_focus_with_viewhelper properly:
- Extracts the request ID from the focus (line 709)
- Resolves the request draft from the view state (line 712)
- Loads the draft into the model (line 713)
This ensures that undoing/redoing actions restores the correct request draft based on the recorded focus.
136-148: TheSplitRatiowrapper in lines 136-148 is necessary. TheSplitRatio::new()method clamps the ratio value to[MIN_SPLIT_RATIO, 1.0 - MIN_SPLIT_RATIO], ensuring invalid states are unrepresentable. This pattern aligns with the coding guideline to embrace ADTs and validate at boundaries, so the code is correct as-is.src/app/history.rs (5)
1-8: LGTM! Constants and imports are well-defined.The snapshot interval (50) and max history (500) are reasonable defaults for balancing memory usage against replay performance.
9-33: LGTM! Clean ADT for focus state.The
Focusenum follows the "make invalid state unrepresentable" principle from the coding guidelines. The helper methods provide ergonomic conversions without exposing internal structure unnecessarily.
136-164:last_focusmay not reflect the correct final focus whentarget == snapshot.index.When
replay_tois called withtarget == snapshot.index, the loopfor entry in self.entries.iter().take(target).skip(snapshot.index)iterates zero times. In this case,last_focusremains equal tosnapshot.focus.clone()(from line 148), which is correct.However, when there are entries to replay,
last_focusis updated inside the loop tocurrent_focus.clone()(line 157) after the action is applied. This meanslast_focuscaptures the focus of the last processed entry, which is the entry at indextarget - 1. This appears intentional—the result focus should be the focus associated with the action that produced the current state.
167-251: Good test coverage for focus-aware replay.The test validates that undo correctly restores focus to the previous request and redo advances to the next. The test helper
apply_focusproperly simulates loading drafts based on focus changes.
76-92: Snapshot retention logic is sound—no stale snapshots after truncation.The concern about snapshots at
index == cursorafter truncation is unfounded. Thesnapshot.indexrepresents the count of entries applied to produce that state. Afterentries.truncate(cursor), we have exactlycursorentries (indexed 0 tocursor-1), making a snapshot atindex=cursorvalid. Thereplay_tologic correctly handles this: when replaying totarget == snapshot.index, it appliestake(target).skip(snapshot.index)(an empty range), using the snapshot model directly. This is the correct behavior and extends consistently to all truncation and replay scenarios.src/app/lifecycle.rs (2)
16-21: Clean separation of concerns with model/view/runtime/history composition.The new structure aligns well with the PR objectives for separating undoable state (
model), UI state (view), and runtime resources (runtime/history). This enables the event-sourced undo/redo design.
60-78: LGTM! Initialization correctly constructs all state components.The flow properly:
- Creates default
AppModel- Builds
ViewStatewith panes and http_root- Updates status with the model
- Persists state
- Initializes
Runtimewith client and state- Creates
Historywith initial model snapshotThis ensures a consistent starting state for undo/redo.
src/app/state.rs (5)
16-30: Good use of newtype pattern forSplitRatio.The clamping in
new()ensures the ratio stays within valid bounds, following the "parse, don't validate" principle from the coding guidelines. This prevents invalid pane splits at construction time.
32-51:EnvironmentIndexprovides compile-time safety for environment access.The
new()constructor validates bounds, andfind()provides a safe lookup. This is a clean ADT approach that makes invalid indices unrepresentable. As per coding guidelines, this embraces ADTs effectively.
137-146: Header rebuilding filters empty names, which is correct behavior.The
filteron line 141 ensures only headers with non-empty names are serialized. This prevents accidentally adding empty headers to the request. The trimming on both name and value is a good defensive measure.
243-259:set_environmentscorrectly handles environment persistence.The method properly:
- Prepends the default environment
- Restores the previously active environment by name if it exists
- Falls back to the default environment
- Persists the active environment name back to state
The
expecton line 251 is safe becausewith_default_environmentguarantees at least one environment exists.
265-276: Theresolve_requestcode is correct as-is.RequestIdis defined as a single-variant enum with onlyHttpFile { path: PathBuf, index: usize }. The refutable pattern will not panic because there are no other variants to handle. This design appropriately makes invalid states unrepresentable, aligning with the ADT principle from the coding guidelines.
| pub fn run() -> iced::Result { | ||
| application(Zagel::init, Zagel::update, view::view) | ||
| .title("Zagel REST workbench") | ||
| .title("Zagel REST workbench") |
There was a problem hiding this comment.
Typo: Double space in window title.
The title "Zagel REST workbench" has two spaces between "Zagel" and "REST".
🔎 Proposed fix
- .title("Zagel REST workbench")
+ .title("Zagel REST workbench")📝 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.
| .title("Zagel REST workbench") | |
| .title("Zagel REST workbench") |
🤖 Prompt for AI Agents
In src/app/lifecycle.rs around line 118, the window title string contains a
double space ("Zagel REST workbench"); change it to a single space ("Zagel REST
workbench") to correct the typo and ensure consistent UI text.
| #[derive(Debug, Clone)] | ||
| pub struct Runtime { | ||
| pub client: Client, | ||
| pub state: AppState, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if Runtime is cloned anywhere in the codebase
rg -n 'runtime\.clone\(\)' src/Repository: sharno/zagel
Length of output: 38
🏁 Script executed:
# Also check for broader clone patterns on Runtime or where Runtime is defined
rg -n 'Runtime' src/app/state.rs | head -20Repository: sharno/zagel
Length of output: 80
🏁 Script executed:
# Check the full content of state.rs to understand AppState and Runtime definitions
wc -l src/app/state.rsRepository: sharno/zagel
Length of output: 76
🏁 Script executed:
# Read the full state.rs file to see Runtime and AppState definitions
cat -n src/app/state.rsRepository: sharno/zagel
Length of output: 10941
🏁 Script executed:
# Search for Runtime usage patterns more broadly (not just clone)
rg -n 'Runtime' src/ -A 2 -B 2Repository: sharno/zagel
Length of output: 991
🏁 Script executed:
# Check if AppState derives Clone
rg -n 'pub struct AppState' src/ -A 5Repository: sharno/zagel
Length of output: 324
🏁 Script executed:
# Check AppState derive attributes and full definition
rg -n 'pub struct AppState' src/state.rs -A 10Repository: sharno/zagel
Length of output: 347
🏁 Script executed:
# Search for any clone() calls on Runtime or its members in the codebase
rg -n '\.clone\(\)' src/app/lifecycle.rs -B 2 -A 2Repository: sharno/zagel
Length of output: 1181
🏁 Script executed:
# Check if AppState actually derives Clone by looking at the full line before line 9
head -20 src/state.rsRepository: sharno/zagel
Length of output: 530
🏁 Script executed:
# Search for the derive macro on AppState
rg -n 'AppState' src/state.rs | head -5Repository: sharno/zagel
Length of output: 98
🏁 Script executed:
# Also check if ThemeChoice derives Clone since it's a field in AppState
rg -n 'enum ThemeChoice' src/ -A 3Repository: sharno/zagel
Length of output: 197
🏁 Script executed:
# Check the full ThemeChoice derive attributes
rg -n 'enum ThemeChoice' src/theme.rs -B 2Repository: sharno/zagel
Length of output: 198
🏁 Script executed:
# Search for actual Runtime.clone() calls in the codebase
rg -n 'runtime\.clone\(\)' src/ -iRepository: sharno/zagel
Length of output: 38
🏁 Script executed:
# Check how Runtime is actually used - is it moved, borrowed, or cloned?
rg -n 'self\.runtime' src/app/lifecycle.rs -B 1 -A 1Repository: sharno/zagel
Length of output: 38
Remove the Clone derive from Runtime unless it's actually cloned.
Runtime is never cloned in the codebase, and it's stored as an owned value in the App struct. AppState explicitly derives Clone, so there's no uncertainty about its cloneability. If Runtime doesn't need to be cloned, remove the derive to keep the API clear about ownership semantics.
🤖 Prompt for AI Agents
In src/app/state.rs around lines 279 to 283, the Runtime struct is derived with
Clone but the type is never cloned and is stored as an owned value in App;
remove the Clone derive from Runtime to reflect correct ownership semantics.
Edit the struct declaration to drop Clone from the #[derive(...)] list (leaving
Debug) and run cargo check to ensure no code relies on Runtime::clone; if any
call sites attempt to clone a Runtime, refactor them to clone the AppState or
otherwise adjust ownership instead of cloning the whole Runtime.
| fn handle_add_request(&mut self) -> Task<Message> { | ||
| if let Some(RequestId::HttpFile { path, .. }) = self.view.selection.clone() { | ||
| let new_draft = RequestDraft { | ||
| title: "New request".to_string(), | ||
| ..Default::default() | ||
| }; | ||
| if let Some(file) = self.view.http_files.get_mut(&path) { | ||
| let persist_draft = new_draft.clone(); | ||
| file.requests.push(new_draft); | ||
| let new_idx = file.requests.len() - 1; | ||
| let new_id = RequestId::HttpFile { | ||
| path: path.clone(), | ||
| index: new_idx, | ||
| }; | ||
| self.view.selection = Some(new_id.clone()); | ||
| if let Some(loaded) = self.view.resolve_request(&new_id) { | ||
| let task = self.apply_model_action(Action::LoadDraft(loaded), false, Some("Ready")); | ||
| self.view.update_response_viewer(); | ||
| return task; | ||
| } | ||
| self.view.update_status_with_model("Saving new request...", &self.model); | ||
| let effect = Effect::PersistRequest { | ||
| root: self.view.http_root.clone(), | ||
| selection: None, | ||
| draft: persist_draft, | ||
| explicit_path: Some(path), | ||
| }; | ||
| return self.apply_model_action(Action::Emit(effect), false, None); | ||
| } | ||
| return Task::none(); | ||
| } | ||
|
|
||
| self.view | ||
| .update_status_with_model("Select a file to add a request", &self.model); | ||
| Task::none() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the handle_saved implementation and understand how file.requests is updated
sed -n '659,700p' src/app/update.rsRepository: sharno/zagel
Length of output: 1600
🏁 Script executed:
# Check if there are any other places where file.requests is modified in response to persist completion
rg -A 10 "fn handle_saved" src/app/update.rsRepository: sharno/zagel
Length of output: 578
🏁 Script executed:
# Look for file rescan logic to understand reconciliation
rg -B 3 -A 10 "rescan\|rescans" src/app/update.rsRepository: sharno/zagel
Length of output: 38
🏁 Script executed:
# Check the full context around handle_add_request and handle_saved to understand the flow
sed -n '564,680p' src/app/update.rsRepository: sharno/zagel
Length of output: 4748
Optimistic state update precedes persistence confirmation.
Line 570 pushes the new request to file.requests before the persist effect completes. While handle_saved calls self.rescan_files() on successful persistence to reconcile state, this only mitigates the issue on success. If persistence fails, the in-memory state remains diverged until the next rescan, creating a temporary inconsistency.
Updating file.requests only in handle_saved after confirming successful persistence (the result's Ok case) would eliminate this window entirely and improve state consistency.
🤖 Prompt for AI Agents
In src/app/update.rs around lines 564-599, the code currently pushes the new
RequestDraft into file.requests and updates selection before the PersistRequest
effect completes; instead, do not mutate in-memory file.requests or change
selection optimistically—create the new draft (persist_draft) and emit
Effect::PersistRequest immediately, and only on a successful save (handle_saved
Ok branch) append the persisted draft to the appropriate file.requests, compute
and set the new RequestId selection there, call rescan_files()/update views as
needed; on failure, leave in-memory state untouched and surface the error
status. Ensure any UI feedback (status/message) is shown before/while persisting
but defer the actual state mutation until persistence confirmation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/app/update.rs (1)
579-619: Optimistic state update precedes persistence confirmation.The new request is pushed to
file.requests(line 587) and selection is updated (line 593) before thePersistRequesteffect completes. If persistence fails, in-memory state remains inconsistent until the nextrescan_files()call inhandle_saved.This concern was raised in a previous review and remains applicable.
🧹 Nitpick comments (1)
src/app/update.rs (1)
412-447: Consider usingdebug_assert!with a condition instead ofdebug_assert!(false, ...).The pattern
debug_assert!(false, "message")unconditionally triggers the assertion in debug builds when reached. While this works, it's clearer to express the invariant directly:- let Some(index) = HeaderIndex::new(idx, len) else { - debug_assert!( - false, - "Invalid header index {idx} for header_rows len {len}" - ); - return Task::none(); - }; + let Some(index) = HeaderIndex::new(idx, len) else { + debug_assert!( + idx < len, + "Invalid header index {idx} for header_rows len {len}" + ); + return Task::none(); + };Alternatively, use
unreachable!()if this branch truly should never be reached, or simply log and return silently if it's a recoverable edge case in production.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/update.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runcargo clippy --all-targets --all-features -- -D warnings(orcargo clippy-strict) before committing to enforce the strict lint baseline
Make invalid state unrepresentable (embrace ADTs - Algebraic Data Types)
Parse, don't validate: parse input at the edges so downstream state stays valid and doesn't need repeated validation
Push ifs up, push fors (loops) down
Prefer immutability and functional programming style when you can without sacrificing code cleanliness
Files:
src/app/update.rs
🧬 Code graph analysis (1)
src/app/update.rs (4)
src/app/options.rs (2)
apply_auth_headers(79-105)build_graphql_body(69-77)src/app/reducer.rs (2)
reduce(61-135)new(14-20)src/app/history.rs (3)
new(63-74)record(76-92)from_selection(16-18)src/app/state.rs (4)
new(22-25)new(36-42)new(172-203)default(85-101)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and test (windows-latest)
- GitHub Check: Build and test (windows-latest)
🔇 Additional comments (11)
src/app/update.rs (11)
1-19: LGTM!Imports are well-organized and the
FILE_SCAN_DEBOUNCEconstant is appropriately defined for debouncing file system events.
21-83: LGTM!Helper functions for edit selection management are well-structured. The index swapping logic correctly handles the bidirectional swap case (a↔b).
501-527: LGTM!The
apply_model_actionmethod cleanly orchestrates the reducer pattern: it captures focus for history, applies the reduction, records the action, updates status, and runs effects. The use ofstd::mem::takeis appropriate here.
529-561: LGTM!The
run_effectsmethod properly batches async tasks for all effect types. Early return on empty effects is efficient.
563-567: LGTM!Simple state persistence helper that properly synchronizes view state to runtime storage.
569-577: LGTM!Selection handling correctly updates view state without recording to history, as selection is view-only state per the PR design.
621-650: LGTM!Request preparation correctly handles GraphQL mode by forcing POST, building the JSON body, and conditionally adding the Content-Type header. Auth headers are applied at the right stage.
652-677: LGTM!Save handling properly distinguishes between updating existing requests (HttpFile selection) and creating new files (with explicit path), with appropriate validation for the new file case.
679-697: LGTM!The
handle_savedhandler correctly updates selection on success and triggers a rescan to reconcile state. Error cases properly surface the failure to the user.
699-725: LGTM!Undo/redo handlers correctly restore model state from history, update the view selection from the recorded focus, and refresh the response viewer. The closure pattern for focus restoration is clean.
728-742: LGTM!Both helper functions are clean and correct. The
headers_contain_namefunction properly handles HTTP header case-insensitivity per RFC 7230.
1 participant
Summary
Testing
Closes #4
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.