change-hook-args #1733
change-hook-args #1733
Conversation
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThis PR refactors hook argument handling across the codebase, replacing the Changes
Sequence Diagram(s)sequenceDiagram
participant Desktop App
participant Tauri
participant Hook System
participant Subprocess
Desktop App->>Tauri: appDataDir()
Tauri-->>Desktop App: data_dir
Desktop App->>Tauri: getName()
Tauri-->>Desktop App: app_name
Desktop App->>Hook System: runEventHooks with resource_dir, app_hyprnote, app_meeting
Hook System->>Hook System: Build CLI args via cli_flag()
Hook System->>Subprocess: Execute with --resource-dir, --app-hyprnote, --app-meeting
Subprocess-->>Hook System: Result
Hook System-->>Desktop App: Completion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 (2)
apps/desktop/src/store/zustand/listener/general.ts (1)
207-222: Before-listening hook runs fire-and-forget; verify ordering expectationsThe
beforeListeningStartedhook is triggered via aPromise.all(...).then(...).catch(...)inside the Effect program but is not awaited, so:
- the session start effect proceeds without waiting for the hook, and
- hook failures are logged but do not affect
start()outcome.If hooks are meant to be advisory, this non-blocking pattern is fine. If you need strict “run hooks successfully before the session is considered started” semantics, consider awaiting
runEventHooks(or integrating it into the Effect program) so failures can influence control flow when appropriate.plugins/hooks/src/docs.rs (1)
310-382: Comprehensive type formatting with union support.The refactored
format_typecorrectly handles TypeScript's type system nuances:
- Null/undefined detection for optional inference (lines 330-332)
- Type deduplication (line 343)
- Recursive union flattening
The union handling block (lines 323-359) is quite involved. If this function grows further, consider extracting it to
format_union_typefor readability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
plugins/hooks/js/bindings.gen.tsis excluded by!**/*.gen.ts
📒 Files selected for processing (10)
apps/desktop/src/store/zustand/listener/general.ts(3 hunks)apps/web/content-collections.ts(1 hunks)apps/web/content/docs/hooks/afterListeningStopped.mdx(1 hunks)apps/web/content/docs/hooks/beforeListeningStarted.mdx(1 hunks)apps/web/src/components/hooks-list.tsx(1 hunks)plugins/hooks/src/config.rs(1 hunks)plugins/hooks/src/docs.rs(5 hunks)plugins/hooks/src/event.rs(2 hunks)plugins/hooks/src/lib.rs(1 hunks)plugins/hooks/src/naming.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
plugins/hooks/src/docs.rs (1)
plugins/hooks/src/naming.rs (1)
cli_flag(1-10)
plugins/hooks/src/event.rs (1)
plugins/hooks/src/naming.rs (1)
cli_flag(1-10)
⏰ 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). (5)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: ci (macos, macos-14)
- GitHub Check: fmt
🔇 Additional comments (16)
plugins/hooks/src/lib.rs (1)
1-7: Newnamingmodule is correctly wired into the crateDeclaring
mod naming;here cleanly exposes the new helper module to the rest of the crate; no issues from a wiring/visibility standpoint.plugins/hooks/src/config.rs (1)
5-20: Doc comment updates are accurate and helpfulThe new comments for
HooksConfigandHookDefinition.commandcorrectly describe the existing fields and behavior without altering the API.apps/web/content/docs/hooks/afterListeningStopped.mdx (1)
3-14: Hook docs align with new argument modelThe updated description and the three arguments (
--resource-dir,--app-hyprnote, optional--app-meeting) match the new hook payload shape and CLI flag naming; this keeps documentation in sync with the implementation.apps/web/content/docs/hooks/beforeListeningStarted.mdx (1)
3-14: Consistent argument documentation for pre-listening hookThe front matter now correctly documents the same three arguments as the post-listening hook, with
--app-meetingmarked optional. This symmetry makes the before/after hooks easy to reason about.plugins/hooks/src/naming.rs (1)
1-32: cli_flag helper and tests look correct and well-scopedThe implementation correctly maps snake_case field names to CLI flags (with
--prefix and_→-), while leaving existing hyphens and empty strings handled as expected. The unit tests capture the intended behavior and key edge cases.apps/web/content-collections.ts (1)
262-274: Hook argument schema extension is backwards compatibleAdding
optional: z.boolean().default(false)to each argument keeps existing docs valid (they default to non-optional) while enabling explicit optional args likeapp-meeting. This aligns with the new UI rendering for optional arguments.apps/desktop/src/store/zustand/listener/general.ts (2)
1-2: New imports are scoped to hook context onlyImporting the app data directory and app name here is appropriate and keeps the hook-related logic self-contained; no conflicts with existing imports.
278-296: After-listening hook mirrors pre-listening behaviorThe
afterListeningStoppedhook uses the same pattern and session-specificresource_dirconstruction as the pre-listening hook, which keeps behavior consistent. As above, it runs non-blocking and only logs failures; that’s reasonable if stop should not be gated on hooks.apps/web/src/components/hooks-list.tsx (1)
12-71: Improved hook + argument layout cleanly surfaces optionalityThe reworked layout (section borders, argument panel, two-column grid) makes hooks and their arguments much easier to scan. Using
arg.optionalto conditionally show an “optional” badge lines up with the updated content schema and MDX front matter.plugins/hooks/src/docs.rs (4)
3-3: LGTM: Clean type refactoring.The introduction of
TypeInfoto bundle type name with optional status is a solid improvement over the previous approach. Theis_falsepredicate for conditional serialization is idiomatic.Also applies to: 33-35, 43-56, 384-386
93-143: LGTM: Hook event parsing is well-structured.The refactored flow through
exported_type_aliases→extract_fields→hook_variants→hook_from_variantis clear and modular. The safe fallbacks (unwrap_or_default,unwrap_or) handle missing types gracefully.
145-176: LGTM: Field extraction properly handles optional detection.The use of
cli_flagat line 157 ensures documentation displays CLI-friendly names (e.g.,--resource-dir), which improves user experience. The combined optional check at line 169 correctly captures both explicit?markers and union-inferred nullability.
195-246: LGTM: AST navigation helpers are robust.These helpers provide safe Option-based navigation through the TypeScript AST. The recursive unwrapping of parenthesized types in
type_lit_from(line 234) is a nice touch.plugins/hooks/src/event.rs (3)
1-1: LGTM: Helper function reduces duplication.The
push_cli_arghelper centralizes the flag-value pair logic and ensures consistent CLI flag formatting throughcli_flag. Keeping it private is appropriate.Also applies to: 32-35
38-61: LGTM: Argument structure and CLI conversion are correct.The capacity hint of 6 at line 51 is optimal (3 fields × 2 elements each). The use of
stringify!combined withcli_flagensures compile-time field name safety with proper CLI formatting.The conditional inclusion of
app_meeting(lines 55-57) correctly handles the optional field.
64-87: LGTM: Struct duplication provides type safety.While
BeforeListeningStartedArgsandAfterListeningStoppedArgsare currently identical, maintaining separate types is sound for:
- Semantic clarity (different lifecycle events)
- Type safety (prevents argument confusion)
- Future flexibility (events may diverge)
No description provided.