Roam: Folder Restructure #77
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request primarily involves a comprehensive reorganization of import paths and directory structures across multiple files in the Roam application. The changes focus on standardizing import statements, renaming directories (such as changing "ResultsView" to "results-view"), and consolidating utility functions. The modifications affect components, utilities, and configuration-related files, aiming to improve code organization and import clarity without altering the core functionality of the application. Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
apps/roam/src/utils/renderNodeConfigPage.ts (2)
Line range hint
28-28: Correct typo in description: 'DEPRACATED' should be 'DEPRECATED'There is a typo in the description of the "Format" option. The word "DEPRACATED" is misspelled and should be "DEPRECATED".
Apply this diff to fix the typo:
- description: `DEPRACATED - Use specification instead. The format ${nodeText} pages should have.`, + description: `DEPRECATED - Use specification instead. The format ${nodeText} pages should have.`,
Line range hint
22-70: Reduce usage of// @ts-ignoreto improve type safetyThe code contains several
// @ts-ignorecomments to suppress TypeScript errors. While this can be a temporary workaround, relying on these comments may hide underlying issues and decrease type safety.Consider addressing the TypeScript errors by:
- Ensuring all components and fields have proper type annotations.
- Updating the
Fieldtypes if necessary.- Using type casting (
as) cautiously where appropriate instead of ignoring errors.This will enhance code reliability and maintainability.
apps/roam/src/index.ts (2)
19-21: Ensure consistent variable naming for stylesThe variable naming for style imports and their corresponding variables is inconsistent:
stylesimported from"./styles/styles.css"assigned tostyle.settingsStylesimported from"./styles/settingsStyles.css"assigned tosettingsStyle.discourseGraphStylesimported from"./styles/discourseGraphStyles.css"assigned todiscourseGraphStyle.Consider aligning the variable names for consistency and clarity.
Apply this diff to rename the variable for consistency:
-const discourseGraphStyle = addStyle(discourseGraphStyles); +const discourseGraphStyles = addStyle(discourseGraphStyles);Update the
elementsarray accordingly:-return { - elements: [style, settingsStyle, discourseGraphStyle], +return { + elements: [style, settingsStyle, discourseGraphStyles],Also applies to: 84-84, 109-109
3-3: Remove redundant import ofgetCurrentUserUidIt appears that
getCurrentUserUidis imported but may not be used elsewhere in the code.Check if
getCurrentUserUidis utilized within this file. If not, consider removing the import to clean up unused code.apps/roam/src/utils/configPageTabs.ts (1)
24-155: Consider modularizing the configuration.The
configPageTabsfunction is quite long and could benefit from being split into smaller, more focused functions.Consider refactoring into separate configuration objects:
const homeConfig = (args: OnloadArgs): ConfigTab => ({ id: "home", fields: [/* ... */] }); const grammarConfig = (args: OnloadArgs): ConfigTab => ({ id: "grammar", fields: [/* ... */] }); const exportConfig = (args: OnloadArgs): ConfigTab => ({ id: "export", fields: [/* ... */] }); export const configPageTabs = (args: OnloadArgs): ConfigTab[] => [ homeConfig(args), grammarConfig(args), exportConfig(args), ];apps/roam/src/utils/registerCommandPaletteCommands.ts (2)
Line range hint
63-63: Address TODO comment about DOM manipulation.The comment indicates a need to replace direct DOM manipulation with a CustomEvent. This would improve the code's maintainability and testability.
Would you like me to help implement the CustomEvent-based solution to replace the setTimeout-based DOM manipulation?
Line range hint
19-142: Consider breaking down the large command registration function.The
registerCommandPaletteCommandsfunction is quite large and handles multiple responsibilities. Consider breaking it down into smaller, focused functions for each command type (export commands, query commands, etc.).Example refactor:
+ const registerExportCommands = (extensionAPI: ExtensionAPI) => { + const exportCurrentPage = () => { /* ... */ }; + const exportDiscourseGraph = async () => { /* ... */ }; + + extensionAPI.ui.commandPalette.addCommand({ + label: "DG: Export - Current Page", + callback: exportCurrentPage, + }); + extensionAPI.ui.commandPalette.addCommand({ + label: "DG: Export - Discourse Graph", + callback: exportDiscourseGraph, + }); + }; + const registerQueryCommands = (extensionAPI: ExtensionAPI) => { + const createQueryBlock = async () => { /* ... */ }; + const refreshCurrentQueryBuilder = () => { /* ... */ }; + + extensionAPI.ui.commandPalette.addCommand({ + label: "DG: Query Block - Create", + callback: createQueryBlock, + }); + extensionAPI.ui.commandPalette.addCommand({ + label: "DG: Query Block - Refresh", + callback: refreshCurrentQueryBuilder, + }); + }; export const registerCommandPaletteCommands = (onloadArgs: OnloadArgs) => { const { extensionAPI } = onloadArgs; - // ... current implementation + registerExportCommands(extensionAPI); + registerQueryCommands(extensionAPI); + // ... register other command groups };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apps/roam/src/components/DiscourseContext.tsx(1 hunks)apps/roam/src/components/QueryBuilder.tsx(1 hunks)apps/roam/src/components/QueryDrawer.tsx(1 hunks)apps/roam/src/components/canvas/Tldraw.tsx(1 hunks)apps/roam/src/components/index.ts(1 hunks)apps/roam/src/components/results-view/ResultsView.tsx(1 hunks)apps/roam/src/components/settings/Settings.tsx(1 hunks)apps/roam/src/index.ts(3 hunks)apps/roam/src/utils/canvasUiOverrides.ts(1 hunks)apps/roam/src/utils/configPageTabs.ts(1 hunks)apps/roam/src/utils/discourseConfigRef.ts(1 hunks)apps/roam/src/utils/getExportSettings.ts(1 hunks)apps/roam/src/utils/initializeObserversAndListeners.ts(2 hunks)apps/roam/src/utils/isDiscourseNodeConfigPage.ts(1 hunks)apps/roam/src/utils/refreshConfigTree.ts(1 hunks)apps/roam/src/utils/registerCommandPaletteCommands.ts(1 hunks)apps/roam/src/utils/renderNodeConfigPage.ts(1 hunks)
🔇 Additional comments (16)
apps/roam/src/components/QueryBuilder.tsx (1)
12-12: LGTM! Import path updated correctly.The import path has been updated to use kebab-case, maintaining consistency with the new directory structure.
apps/roam/src/components/index.ts (1)
20-21: Verify Timeline component's new location.The export paths have been updated to match the new directory structure. However, the Timeline component's path has been simplified from
./ResultsView/views/Timelineto./results-view/Timeline.Let's verify the Timeline component's new location:
✅ Verification successful
Timeline component location verified
The Timeline component is correctly located at
apps/roam/src/components/results-view/Timeline.tsx, matching the updated export path.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the Timeline component exists in the new location # Test: Check if Timeline.tsx exists in the results-view directory fd -e tsx "Timeline" "apps/roam/src/components/results-view"Length of output: 109
apps/roam/src/components/QueryDrawer.tsx (1)
19-19: LGTM! Import path updated correctly.The import path has been updated to use kebab-case, maintaining consistency with the new directory structure.
apps/roam/src/components/DiscourseContext.tsx (1)
23-23: LGTM! Verify all ResultsView imports are updated.The import path has been updated to use kebab-case, maintaining consistency with the new directory structure.
Let's verify that all ResultsView imports have been updated across the codebase:
✅ Verification successful
All ResultsView imports are correctly updated
All 4 files importing ResultsView are using the new kebab-case path format (
results-view/ResultsView). No instances of the old PascalCase path format were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining old-style ResultsView imports # Test: Search for imports using the old PascalCase path. Expect: No matches. # Check for old import paths rg -l "from ['\"].*ResultsView/ResultsView['\"]" # Check for new import paths to confirm the changes rg -l "from ['\"].*results-view/ResultsView['\"]"Length of output: 273
apps/roam/src/utils/canvasUiOverrides.ts (1)
16-18: Import paths have been standardized to use absolute paths.The change from relative to absolute imports using the "~/" prefix improves maintainability and reduces the risk of path traversal issues.
Let's verify that the new import paths exist:
Also applies to: 24-24
✅ Verification successful
Import paths and symbols verified successfully
All imported files exist at their new locations and export the required symbols correctly. The standardization to absolute imports using "~/" prefix is properly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the imported files exist at their new locations # Check for the imported files fd -t f "Tldraw.tsx|DiscourseNodeCanvasSettings.tsx|CanvasDrawer.tsx|DiscourseRelationsUtil.tsx" apps/roam/src/components/canvas apps/roam/src/components/settingsLength of output: 381
Script:
#!/bin/bash # Check for the specific exports in the files # Check for COLOR_ARRAY and discourseContext in Tldraw.tsx echo "=== Checking Tldraw.tsx exports ===" ast-grep --pattern 'export const COLOR_ARRAY' apps/roam/src/components/canvas/Tldraw.tsx ast-grep --pattern 'export const discourseContext' apps/roam/src/components/canvas/Tldraw.tsx # Check for formatHexColor in DiscourseNodeCanvasSettings.tsx echo -e "\n=== Checking DiscourseNodeCanvasSettings.tsx exports ===" ast-grep --pattern 'export const formatHexColor' apps/roam/src/components/settings/DiscourseNodeCanvasSettings.tsx # Check for openCanvasDrawer in CanvasDrawer.tsx echo -e "\n=== Checking CanvasDrawer.tsx exports ===" ast-grep --pattern 'export const openCanvasDrawer' apps/roam/src/components/canvas/CanvasDrawer.tsxLength of output: 4618
apps/roam/src/components/canvas/Tldraw.tsx (1)
50-51: Import paths standardized to use absolute paths.The change from relative to absolute imports aligns with the folder restructuring effort and improves code organization by properly separating utilities.
Let's verify that the utility files exist in their new location:
✅ Verification successful
Import paths verified and correctly standardized
Both utility files exist in their expected locations under the utils directory, confirming that the absolute import paths are valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the imported utility files exist # Check for the imported files fd -t f "useRoamStore.ts|canvasUiOverrides.ts" apps/roam/src/utilsLength of output: 145
apps/roam/src/components/results-view/ResultsView.tsx (1)
30-32: Import paths updated for co-located components.The change to use relative imports for closely related components is appropriate. The directory name change to kebab-case (
results-view) follows common naming conventions.Let's verify that the imported components exist in their new locations:
Also applies to: 39-39
✅ Verification successful
All imported components verified in correct location
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the imported component files exist # Check for the imported files fd -t f "Charts.tsx|Timeline.tsx|Kanban.tsx|ResultsTable.tsx" apps/roam/src/components/results-viewLength of output: 305
apps/roam/src/utils/refreshConfigTree.ts (1)
8-8: Import path updated correctlyThe import path for
DISCOURSE_CONFIG_PAGE_TITLEhas been updated to"~/utils/renderNodeConfigPage", which aligns with the recent changes in file structure.apps/roam/src/index.ts (1)
11-12: Updated import paths reflect folder restructuringThe import paths for
registerCommandPaletteCommandsandcreateSettingsPanelhave been updated to reflect their new locations in theutilsdirectory. This change aligns with the project's folder restructuring and improves organization.apps/roam/src/components/settings/Settings.tsx (1)
9-9: LGTM! Import path change is consistent with restructuring.The updated import path aligns with the folder restructuring objectives.
apps/roam/src/utils/initializeObserversAndListeners.ts (2)
14-14: LGTM! Import path changes are consistent with restructuring.The updated import paths align with the folder restructuring objectives.
Also applies to: 36-36
Line range hint
108-110: Document the status of the grammar overlay feature.The TODO comment about grammar overlay refactoring needs more context.
Consider:
- Adding a reference to the tracking issue
- Documenting the expected timeline
- Adding more context about the refactoring scope
- // TODO: grammar overlay being refactored + // TODO: Grammar overlay is being refactored as part of issue #<number> + // Expected completion: Q1 2024 + // Scope: Improving performance and fixing overlay positioning issues // if (isFlagEnabled("grammar.overlay")) { // addPageRefObserver((s) => overlayPageRefHandler(s, onloadArgs)); // }✅ Verification successful
Document the current status of the grammar overlay feature
The TODO comment should be updated to reflect the current status, but since there's no tracking issue found, here's a more accurate documentation:
- // TODO: grammar overlay being refactored + // NOTE: Grammar overlay feature is currently disabled and being reworked + // Status: Intentionally disabled in UI config (see configPageTabs.ts) + // Purpose: Overlays discourse context information over node references // if (isFlagEnabled("grammar.overlay")) { // addPageRefObserver((s) => overlayPageRefHandler(s, onloadArgs)); // }🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for mentions of grammar overlay in the codebase echo "=== Searching for grammar overlay mentions ===" rg -i "grammar.?overlay" -A 2 -B 2 echo -e "\n=== Searching for related TODO comments ===" rg -i "TODO.*grammar" -A 2 -B 2 echo -e "\n=== Searching for feature flag related to grammar overlay ===" rg "isFlagEnabled.*grammar" -A 2 -B 2 echo -e "\n=== Looking for documentation files ===" fd -e md -e mdx | xargs rg -i "grammar.?overlay"Length of output: 2205
Script:
#!/bin/bash # Search for the handler implementation echo "=== Searching for overlayPageRefHandler ===" rg "overlayPageRefHandler" -A 5 -B 5 echo -e "\n=== Looking for recent git history about grammar overlay ===" git log --oneline -n 10 | grep -i "grammar" echo -e "\n=== Searching for feature flag definitions ===" rg "grammar.overlay.*=|GRAMMAR_OVERLAY" --type-add 'config:*.{json,js,ts}' -t configLength of output: 4026
Script:
#!/bin/bash # Search for related issues echo "=== Searching for related GitHub issues ===" gh issue list --search "grammar overlay in:title,body" --state all --limit 5 || true gh pr list --search "grammar overlay in:title,body" --state all --limit 5 || trueLength of output: 251
apps/roam/src/utils/isDiscourseNodeConfigPage.ts (1)
1-1: LGTM! Import path change is consistent with restructuring.The updated import path aligns with the folder restructuring objectives.
apps/roam/src/utils/discourseConfigRef.ts (1)
10-10: Consider separating configuration constants from rendering logic.The constant
DISCOURSE_CONFIG_PAGE_TITLEis now imported fromrenderNodeConfigPage, but this mixing of concerns (configuration constants with rendering logic) might affect maintainability. Consider moving configuration-related constants to a dedicated configuration file.Let's verify if there are other configuration constants mixed with rendering logic:
apps/roam/src/utils/getExportSettings.ts (1)
5-5: LGTM! Consistent with folder restructuring.The import path change aligns with the folder restructuring effort and maintains consistency with other files.
Let's verify the usage of this constant across the codebase to ensure all imports are updated:
✅ Verification successful
Import path changes verified and consistent
All files importing
DISCOURSE_CONFIG_PAGE_TITLEare using the new path~/utils/renderNodeConfigPage, and no instances of the old import path remain in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining old imports rg "from ['\"]~/settings/configPages['\"]"Length of output: 48
Script:
#!/bin/bash # Search for all occurrences of the constant rg "DISCOURSE_CONFIG_PAGE_TITLE" -A 2 # Search for imports from the new path rg "from ['\"]~/utils/renderNodeConfigPage['\"]"Length of output: 3709
apps/roam/src/utils/registerCommandPaletteCommands.ts (1)
12-13: LGTM! Simplified import paths.The change from relative paths (
../utils/) to current directory paths (./) improves clarity and maintainability.
Summary by CodeRabbit
Refactor
Chores
Note: These changes primarily involve internal code organization and do not introduce new user-facing features or functionality.