Conversation
Features: - Daily reminder at 5pm (user timezone) via proactive messaging - Bot commands: help, subscribe, unsubscribe, status, remind me at [time] - Adaptive Cards for welcome, reminder, and status displays - Embedded tabs for Timesheet, Timer, and Reports from thyme.knowall.ai - Azure Table Storage for subscription persistence - Azure Function timer trigger for hourly reminder checks Tech stack: - Teams Toolkit for provisioning and deployment - Microsoft Bot Framework SDK 4.x - TypeScript with Express.js - Azure App Service hosting - Azure Functions for scheduled tasks Includes: - Complete project structure following Teams Toolkit patterns - Bicep templates for Azure infrastructure - Unit tests for bot, cards, and reminder service - AI assistant personas (AGENTS.md) for development - VS Code debugging configuration
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive Microsoft Teams bot application for daily timesheet reminders. The bot integrates with the Thyme timesheet system (thyme.knowall.ai) to send proactive reminders at 5pm in each user's local timezone, with support for custom reminder times.
Key changes:
- Bot Framework implementation with command handlers (help, subscribe, unsubscribe, status, custom time)
- Azure infrastructure provisioning via Bicep templates for App Service, Functions, and Table Storage
- Proactive messaging system using Azure Functions timer triggers running hourly to check for users needing reminders
- Three embedded tabs (Timesheet, Timer, Reports) directly from the Thyme web application
Reviewed changes
Copilot reviewed 45 out of 50 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| src/bot.ts | Main bot activity handler with message routing and command processing |
| src/services/reminderService.ts | Proactive messaging logic with timezone-aware reminder scheduling |
| src/services/subscriptionStore.ts | Azure Table Storage integration for subscription persistence |
| src/services/thymeApiClient.ts | HTTP client for fetching user timesheet data from Thyme API |
| src/commands/*.ts | Command handlers for help, subscribe, unsubscribe, status operations |
| src/cards/*.ts | Adaptive Card definitions for welcome, reminder, and status displays |
| src/config.ts | Environment variable configuration management |
| src/index.ts | Express server setup with Bot Framework adapter |
| functions/dailyReminder/index.ts | Azure Function timer trigger for hourly reminder checks |
| infra/azure.bicep | Infrastructure as Code defining Azure resources |
| tests/*.test.ts | Unit tests for bot, cards, and reminder service |
| package.json | Project dependencies and npm scripts |
| tsconfig.json | TypeScript compiler configuration |
| appPackage/manifest.json | Teams app manifest with bot and tab definitions |
| teamsapp.yml | Teams Toolkit provisioning and deployment configuration |
| README.md | Comprehensive project documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function getTimezoneFromLocale(locale: string): string { | ||
| // Simplified timezone mapping based on locale | ||
| const timezoneMap: Record<string, string> = { | ||
| "en-GB": "Europe/London", | ||
| "en-US": "America/New_York", |
There was a problem hiding this comment.
The getTimezoneFromLocale function assumes all US users are in the Eastern timezone ('America/New_York'), which is incorrect. The US spans multiple timezones (Pacific, Mountain, Central, Eastern, Alaska, Hawaii). This will result in incorrect reminder times for users in other US timezones. Consider either asking users to set their timezone explicitly or using a more sophisticated timezone detection method.
| } catch (error) { | ||
| // Table may already exist - ignore error | ||
| const err = error as { statusCode?: number }; | ||
| if (err.statusCode !== 409) { |
There was a problem hiding this comment.
The error handling catches errors but doesn't check if err.statusCode exists before accessing it. While the type assertion helps, if the error doesn't have a statusCode property, this will fail silently. Consider checking if the property exists before accessing it or using optional chaining.
| param thymeApiUrl string = 'https://thyme.knowall.ai/api' | ||
|
|
||
| // Storage Account for subscriptions | ||
| resource storageAccount 'Microsoft.Storage/storageAccounts@2023-01-01' = { |
There was a problem hiding this comment.
The API version for Microsoft.Storage/storageAccounts is '2023-01-01', but this may not be the latest stable version. Consider using the most recent stable API version to ensure access to the latest features and security updates. As of knowledge cutoff (Jan 2025), there may be newer versions available.
| const timerTrigger: AzureFunction = async (context: Context): Promise<void> => { | ||
| const timestamp = new Date().toISOString(); | ||
| context.log(`Daily reminder function started at ${timestamp}`); | ||
|
|
||
| try { | ||
| const reminderService = new ReminderService(); |
There was a problem hiding this comment.
The function unconditionally creates new instances of ReminderService on every timer invocation. Since ReminderService creates a CloudAdapter, SubscriptionStore, and ThymeApiClient in its constructor, this could lead to resource leaks if these objects aren't properly disposed. Consider reusing service instances or implementing proper cleanup.
| const timerTrigger: AzureFunction = async (context: Context): Promise<void> => { | |
| const timestamp = new Date().toISOString(); | |
| context.log(`Daily reminder function started at ${timestamp}`); | |
| try { | |
| const reminderService = new ReminderService(); | |
| const reminderService = new ReminderService(); | |
| const timerTrigger: AzureFunction = async (context: Context): Promise<void> => { | |
| const timestamp = new Date().toISOString(); | |
| context.log(`Daily reminder function started at ${timestamp}`); | |
| try { |
| botId: getEnvVar("BOT_ID", ""), | ||
| botPassword: getEnvVar("BOT_PASSWORD", ""), |
There was a problem hiding this comment.
The configuration allows empty strings for required bot credentials (BOT_ID and BOT_PASSWORD) when no default is specified. If these values are empty, the bot will fail at runtime when trying to authenticate. Consider validating that these critical values are non-empty strings and throwing an error during configuration loading if they're missing.
| const todayStatus = hoursToday > 0 ? `${hoursToday.toFixed(1)} hours` : "No hours logged"; | ||
| const weekStatus = hoursThisWeek > 0 ? `${hoursThisWeek.toFixed(1)} hours` : "No hours logged"; |
There was a problem hiding this comment.
The API returns "No hours logged" when hours are 0, but this message could be more encouraging or actionable for the user. Consider a message like "No hours logged yet" or "Start logging your hours" to be more engaging and prompt action.
| resource storageAccount 'Microsoft.Storage/storageAccounts@2023-01-01' = { | ||
| name: toLower(replace('${resourceBaseName}storage', '-', '')) |
There was a problem hiding this comment.
The storage account name is created by removing hyphens from resourceBaseName, but Azure Storage account names must be 3-24 characters long and can only contain lowercase letters and numbers. If resourceBaseName is too long or contains uppercase letters, this could fail. Consider adding validation or using a substring with toLower() to ensure compliance with Azure naming requirements.
| resource storageAccount 'Microsoft.Storage/storageAccounts@2023-01-01' = { | |
| name: toLower(replace('${resourceBaseName}storage', '-', '')) | |
| var storageAccountNameBase = toLower(replace('${resourceBaseName}storage', '-', '')) | |
| var storageAccountName = length(storageAccountNameBase) > 24 ? substring(storageAccountNameBase, 0, 24) : storageAccountNameBase | |
| resource storageAccount 'Microsoft.Storage/storageAccounts@2023-01-01' = { | |
| name: storageAccountName |
| @@ -0,0 +1,135 @@ | |||
| import { TurnContext, TeamsInfo, CardFactory, MessageFactory, ConversationReference } from "botbuilder"; | |||
There was a problem hiding this comment.
Unused import ConversationReference.
| @@ -0,0 +1,153 @@ | |||
| import { TurnContext, Activity, ConversationReference, ChannelAccount } from "botbuilder"; | |||
There was a problem hiding this comment.
Unused import ConversationReference.
|
|
||
| describe("sendDailyReminders", () => { | ||
| it("should process all eligible subscriptions", async () => { | ||
| const mockSubs: Subscription[] = [ |
There was a problem hiding this comment.
Unused variable mockSubs.
CI/CD: - Add GitHub Actions CI workflow (build, test, lint, format check) - Add GitHub Actions CD workflow for Azure deployment - Add PR check workflow with validation and labeling - Add PR template and labeler configuration Code Quality: - Add Prettier configuration and eslint-prettier integration - Add format, format:check, typecheck, and ci scripts - Fix reminder service time window logic (15-min window instead of 60) - Update .gitignore for sensitive files Infrastructure: - Add web.config for Azure App Service IIS deployment
- Add AbortController with 10s timeout to all fetch requests in ThymeApiClient - Add fetchWithTimeout helper method for consistent timeout handling - Add specific error handling for timeout (AbortError) vs other errors - Add try-catch wrapper to /api/messages endpoint with proper response handling
CI: - Update codecov/codecov-action from v3 to v4 - Add CODECOV_TOKEN environment variable (required for v4) CD: - Update azure/login from v1 to v2 - Update azure/webapps-deploy from v2 to v3 - Add deployment package preparation with node_modules - Fix functions build to include shared code dependencies - Add functions package-lock.json for reproducible builds - Prepare proper deployment artifacts with production dependencies
- Update function.json scriptFile to "./index.js" for correct deployment path - Fix CD workflow to copy compiled JS files into function folder alongside function.json - Azure Functions expects: /dailyReminder/function.json + /dailyReminder/index.js
- Fix timezone locale heuristic with expanded mappings and documentation - Add optional chaining for statusCode checks in subscriptionStore - Update Azure API versions (Storage 2023-05-01, Web 2023-12-01, Bot 2023-09-15) - Move ReminderService to module level for cold start optimization - Add bot credentials validation with warning for local development - Update Bicep CLI version from v0.9.1 to v0.24.24 - Fix Bot Framework authentication configuration - Fix TypeScript errors in tests
- Add @minlength(3) and @maxlength(17) to resourceBaseName parameter - Use take() function to ensure storage name stays under 24 chars - Addresses Azure storage account naming requirements (3-24 chars)
- ThymeApiClient: use singleton pattern for efficiency
- Grammar: change contractions to formal language ("could not")
- Tests: move jest.mock to module level with jest.spyOn
- Rate limiting: add documentation about Teams limits
- Monday calculation: add detailed ISO week comments
BenGWeeks
left a comment
There was a problem hiding this comment.
Copilot Feedback Responses
All critical issues have been addressed in commits b479771, 04f6896, and 13bc199.
Resolved with Code Changes:
- Timezone detection: Expanded locale mappings, changed en-US to Central, added documentation
- statusCode optional chaining: Changed to
err?.statusCodein both locations - Azure API versions: Updated Storage to 2023-05-01, Web to 2023-12-01, BotService to 2023-09-15
- ReminderService per-invocation: Moved to module level for cold start optimization
- Bot credentials validation: Added
validateBotCredentials()with startup warning - Bicep CLI version: Updated from v0.9.1 to v0.24.24
- Storage account naming: Added @minLength/@maxlength and take() function
- ThymeApiClient singleton: Changed to module-level singleton pattern
- Grammar: Changed contractions to formal language
- jest.mock placement: Moved to module level using jest.spyOn
- Rate limiting docs: Added detailed comments about Teams limits
- Monday calculation docs: Added ISO week documentation
Already Correct / No Change Needed:
- Test assertions: Filtering tested separately in isReminderTime tests
- OData escaping: Values generated internally, Azure SDK handles escaping
- userPrincipalName: Populated by TeamsInfo.getMember() before subscription
- Test dates: Tests use DateTime.now() dynamically
- Minute range: Already fixed to 0-15 in earlier commit
- Weekend handling: Standard JS convention (0=Sunday, 6=Saturday)
- Teams channel: Correct Bicep syntax per Azure ARM schema
- No hours message: Card includes Log Time action button
Features:
Tech stack:
Includes: