-
Notifications
You must be signed in to change notification settings - Fork 239
transcript option upgrade #1294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughA new feature was added to the Share button in the desktop app, allowing users to optionally include a formatted transcript when exporting notes to Obsidian. This includes UI changes for toggling transcript inclusion, updates to the export handler, and new helper functions for transcript formatting and speaker labeling. Additionally, inline comments were added to clarify participant synchronization logic in the note route without changing functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ShareButtonInNote
participant ExportHandlers
participant ObsidianExport
participant Helpers
User->>ShareButtonInNote: Clicks Share Button
ShareButtonInNote->>ShareButtonInNote: Shows Obsidian export UI with transcript checkbox
User->>ShareButtonInNote: Selects folder, toggles "Include Transcript"
ShareButtonInNote->>ExportHandlers: Calls obsidian(note, folder, includeTranscript)
ExportHandlers->>ObsidianExport: Prepares markdown export
alt includeTranscript is true
ObsidianExport->>Helpers: convertWordsToTranscript(words)
Helpers->>ObsidianExport: Returns formatted transcript
ObsidianExport->>ExportHandlers: Appends transcript to markdown
end
ExportHandlers->>ShareButtonInNote: Completes export
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/components/toolbar/buttons/share-button.tsx
(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit Configuration File
**/*.{js,ts,tsx,rs}
: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
apps/desktop/src/components/toolbar/buttons/share-button.tsx
🔇 Additional comments (4)
apps/desktop/src/components/toolbar/buttons/share-button.tsx (4)
41-41
: LGTM!The new state variable is properly initialized and follows React conventions.
189-189
: LGTM!The
includeTranscript
parameter is correctly passed through and has a proper default value.Also applies to: 448-448
325-359
: LGTM!The UI implementation for the transcript toggle is well-structured with proper labeling and state management.
471-479
: LGTM!The conditional transcript appending logic is correct and properly checks for both the flag and the existence of word data before processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cubic analysis
1 issue found across 1 file • Review in cubic
React with 👍 or 👎 to teach cubic. You can also tag @cubic-dev-ai
to give feedback, ask questions, or re-run the review.
|| (currentSpeaker?.type === "unassigned" && word.speaker?.type === "unassigned" | ||
&& currentSpeaker.value.index === word.speaker.value.index) | ||
|| (currentSpeaker?.type === "assigned" && word.speaker?.type === "assigned" | ||
&& currentSpeaker.value.id === word.speaker.value.id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currentSpeaker.value or word.speaker.value can be undefined, so accessing .id risks a runtime crash. Add optional-chaining before dereferencing.
Prompt for AI agents
Address the following comment on apps/desktop/src/components/toolbar/buttons/share-button.tsx at line 604:
<comment>currentSpeaker.value or word.speaker.value can be undefined, so accessing .id risks a runtime crash. Add optional-chaining before dereferencing.</comment>
<file context>
@@ -561,3 +586,59 @@ async function fetchObsidianFolders(): Promise<ObsidianFolder[]> {
return [{ value: "default", label: "Default (Root)" }];
}
}
+
+function convertWordsToTranscript(words: any[]): string {
+ if (!words || words.length === 0) {
+ return "";
+ }
+
</file context>
&& currentSpeaker.value.id === word.speaker.value.id); | |
&& currentSpeaker?.value?.id === word.speaker?.value?.id); |
Summary by cubic
Added an "Include transcript" option when exporting notes to Obsidian, allowing users to attach the full session transcript to the exported markdown.