feat: Use SpaceTemplate info for Namespace#739
Conversation
- Get space template by appid and use the namespace from the space template instead of from the app - Getting the app requires account level access, but agents only have environment level access - Also fixed a bug with space update request body
WalkthroughThe pull request modifies the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
plugins/space-configure-from-template/src/space.configure.from.template.ts (4)
109-111: Enhance error handling for better debugging.Consider preserving the original error message when re-throwing and use a proper logging solution instead of console.log.
- console.log('Space configuration failed with error:', error) - throw new Error('Space configuration failed') + console.error('Space configuration failed:', error) + throw new Error(`Space configuration failed: ${error.message}`)
58-72: Add validation for required workbook properties.Consider adding validation for required properties before creating workbook configs to prevent potential runtime errors.
const workbookConfigs: Flatfile.CreateWorkbookConfig[] = workbooks.data.map((workbook) => ({ + // Validate required properties + ...(!workbook.name && { name: 'Untitled Workbook' }), name: workbook.name, labels: workbook.labels, spaceId: spaceId, environmentId: environmentId, namespace: spaceTemplate.namespace, sheets: workbook.sheets.map((sheet) => ({ + // Validate sheet config + ...(!sheet.config && { config: {} }), ...sheet.config, })), actions: workbook.actions, settings: workbook.settings, metadata: workbook.metadata, treatments: workbook.treatments, }))
85-95: Simplify primaryWorkbookId assignment.The conditional expression for primaryWorkbookId can be simplified.
await api.spaces.update(spaceId, { - primaryWorkbookId: - workbookIds && workbookIds.length > 0 ? workbookIds[0] : '', + primaryWorkbookId: workbookIds[0] || '', settings: spaceTemplate.settings || {}, metadata: spaceTemplate.metadata, actions: spaceTemplate.actions || [], access: spaceTemplate.access || [], labels: spaceTemplate.labels, translationsPath: spaceTemplate.translationsPath || '', languageOverride: spaceTemplate.languageOverride || '', })
97-103: Optimize document creation and add progress tracking.Consider using batched operations for document creation and add progress tracking using the tick function.
const documents = await api.documents.list(spaceTemplateId) +const totalDocuments = documents.data.length +let processedDocuments = 0 -for (const document of documents.data) { - await api.documents.create(spaceId, document) -} +const batchSize = 10 +for (let i = 0; i < documents.data.length; i += batchSize) { + const batch = documents.data.slice(i, i + batchSize) + await Promise.all(batch.map(doc => api.documents.create(spaceId, doc))) + processedDocuments += batch.length + await tick(75 + (processedDocuments / totalDocuments) * 20, + `Created ${processedDocuments}/${totalDocuments} documents`) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonplugins/space-configure-from-template/package.jsonis excluded by!**/*.json
📒 Files selected for processing (1)
plugins/space-configure-from-template/src/space.configure.from.template.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Setup
- GitHub Check: Lint
- GitHub Check: Nullify Dependencies
🔇 Additional comments (3)
plugins/space-configure-from-template/src/space.configure.from.template.ts (3)
48-49: Verify template selection logic.The code always selects the oldest template. Is this the intended behavior? Consider adding a way to specify which template to use, perhaps through metadata or labels.
105-108: LGTM! Callback execution looks good.The callback handling is implemented correctly with all necessary parameters.
36-42: Consider handling pagination in space templates list.The current implementation might not handle cases where there are many space templates. Consider adding pagination parameters and handling multiple pages.
plugins/space-configure-from-template/src/space.configure.from.template.ts
Show resolved
Hide resolved
2 participants
Please explain how to summarize this PR for the Changelog:
Tell code reviewer how and what to test: